| Message ID | 2179bf7c-9878-adf7-da97-2746d5aa3d34@users.sourceforge.net |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
Hi Markus, On Sun, Sep 4, 2016 at 2:38 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 3 Sep 2016 17:45:28 +0200 > > Move the assignments for four local variables a bit at the beginning > so that they will only be performed if a corresponding memory allocation > succeeded by this function. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > arch/sparc/net/bpf_jit_comp.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c > index ced1393..a927470 100644 > --- a/arch/sparc/net/bpf_jit_comp.c > +++ b/arch/sparc/net/bpf_jit_comp.c > @@ -362,10 +362,10 @@ do { *prog++ = BR_OPC | WDISP22(OFF); \ > > void bpf_jit_compile(struct bpf_prog *fp) > { > - unsigned int cleanup_addr, proglen, oldproglen = 0; > - u32 temp[8], *prog, *func, seen = 0, pass; > - const struct sock_filter *filter = fp->insns; > - int i, flen = fp->len, pc_ret0 = -1; > + unsigned int cleanup_addr, proglen, oldproglen; > + u32 temp[8], *prog, *func, seen, pass; > + const struct sock_filter *filter; > + int i, flen = fp->len, pc_ret0; > unsigned int *addrs; > void *image; > > @@ -385,6 +385,10 @@ void bpf_jit_compile(struct bpf_prog *fp) > } > cleanup_addr = proglen; /* epilogue address */ > image = NULL; > + filter = fp->insns; > + oldproglen = 0; > + pc_ret0 = -1; > + seen = 0; > for (pass = 0; pass < 10; pass++) { > u8 seen_or_pass0 = (pass == 0) ? (SEEN_XREG | SEEN_DATAREF | SEEN_MEM) : seen; This is utterly pointless, why? If you were moving the assignments on declaration onto separate lines at the top of the file then ok, but why all the way down here? Thanks,
>> Date: Sat, 3 Sep 2016 17:45:28 +0200 >> >> Move the assignments for four local variables a bit at the beginning >> so that they will only be performed if a corresponding memory allocation >> succeeded by this function. … >> @@ -362,10 +362,10 @@ do { *prog++ = BR_OPC | WDISP22(OFF); \ >> >> void bpf_jit_compile(struct bpf_prog *fp) >> { >> - unsigned int cleanup_addr, proglen, oldproglen = 0; >> - u32 temp[8], *prog, *func, seen = 0, pass; >> - const struct sock_filter *filter = fp->insns; >> - int i, flen = fp->len, pc_ret0 = -1; >> + unsigned int cleanup_addr, proglen, oldproglen; >> + u32 temp[8], *prog, *func, seen, pass; >> + const struct sock_filter *filter; >> + int i, flen = fp->len, pc_ret0; >> unsigned int *addrs; >> void *image; >> >> @@ -385,6 +385,10 @@ void bpf_jit_compile(struct bpf_prog *fp) >> } >> cleanup_addr = proglen; /* epilogue address */ >> image = NULL; >> + filter = fp->insns; >> + oldproglen = 0; >> + pc_ret0 = -1; >> + seen = 0; >> for (pass = 0; pass < 10; pass++) { >> u8 seen_or_pass0 = (pass == 0) ? (SEEN_XREG | SEEN_DATAREF | SEEN_MEM) : seen; … > If you were moving the assignments on declaration onto separate lines > at the top of the file then ok, I see another software design option where the transformation result might be looking more pleasing for you again. > but why all the way down here? * How do you think about the reason I gave in the short commit message? * Are you interested in an other software refactoring instead? http://refactoring.com/catalog/reduceScopeOfVariable.html Would you eventually like to move the source code for this for loop into another function? http://refactoring.com/catalog/extractMethod.html Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On Sun, Sep 4, 2016 at 2:33 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> Date: Sat, 3 Sep 2016 17:45:28 +0200 >>> >>> Move the assignments for four local variables a bit at the beginning >>> so that they will only be performed if a corresponding memory allocation >>> succeeded by this function. > … >>> @@ -362,10 +362,10 @@ do { *prog++ = BR_OPC | WDISP22(OFF); \ >>> >>> void bpf_jit_compile(struct bpf_prog *fp) >>> { >>> - unsigned int cleanup_addr, proglen, oldproglen = 0; >>> - u32 temp[8], *prog, *func, seen = 0, pass; >>> - const struct sock_filter *filter = fp->insns; >>> - int i, flen = fp->len, pc_ret0 = -1; >>> + unsigned int cleanup_addr, proglen, oldproglen; >>> + u32 temp[8], *prog, *func, seen, pass; >>> + const struct sock_filter *filter; >>> + int i, flen = fp->len, pc_ret0; >>> unsigned int *addrs; >>> void *image; >>> >>> @@ -385,6 +385,10 @@ void bpf_jit_compile(struct bpf_prog *fp) >>> } >>> cleanup_addr = proglen; /* epilogue address */ >>> image = NULL; >>> + filter = fp->insns; >>> + oldproglen = 0; >>> + pc_ret0 = -1; >>> + seen = 0; >>> for (pass = 0; pass < 10; pass++) { >>> u8 seen_or_pass0 = (pass == 0) ? (SEEN_XREG | SEEN_DATAREF | SEEN_MEM) : seen; > … >> If you were moving the assignments on declaration onto separate lines >> at the top of the file then ok, > > I see another software design option where the transformation result might be looking > more pleasing for you again. > > >> but why all the way down here? > > * How do you think about the reason I gave in the short commit message? Does this change improve the resulting binary? I.e. does it make it smaller or faster? If it's smaller, by how much? if it's faster, measure it. Otherwise this change is useless churn - you're making the code more complicated, longer and harder to read for practically no benefit. Thanks,
> Does this change improve the resulting binary? I hope so. - I propose to give the refactorings "Reduce scope of variable" and "Extract a function" (and the corresponding consequences) another look. > I.e. does it make it smaller or faster? It is generally possible that a specific code generation variant will also affect the run time properties you mentioned. > Otherwise this change is useless churn - you're making the code more > complicated, longer and harder to read for practically no benefit. I imagine that there other reasons you could eventually accept for this use case, aren't there? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On Sun, Sep 4, 2016 at 3:00 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >> Does this change improve the resulting binary? > > I hope so. - I propose to give the refactorings "Reduce scope of variable" > and "Extract a function" (and the corresponding consequences) another look. So you _think_ it does. Come back with real proof. I must also point out that these sorts of optimisations are things the compiler does automatically when compiling this code. Therefore it's highly likely that this change will make absolutely no difference whatsoever. (And no it won't improve compile speed in any justifiable way) >> I.e. does it make it smaller or faster? > > It is generally possible that a specific code generation variant will also affect > the run time properties you mentioned. It's _possible_? Come back with benchmarks. I must also point out that this is a "slow path" i.e. as long as it's not stupidly inefficient, the speed doesn't matter that much. This change isn't going to improve the speed of this function by any amount that matters. >> Otherwise this change is useless churn - you're making the code more >> complicated, longer and harder to read for practically no benefit. > > I imagine that there other reasons you could eventually accept > for this use case, aren't there? Unless you have some pretty damn good proof that these changes improve things, there is absolutely no reason to take them as-is - you are making the code longer and more difficult to read for no benefit and wasting everyone's time in the process. Thanks,
>> I hope so. - I propose to give the refactorings "Reduce scope of variable" >> and "Extract a function" (and the corresponding consequences) another look. > > So you _think_ it does. Come back with real proof. Which test environments would you find acceptable for further clarification? > I must also point out that these sorts of optimisations are things the > compiler does automatically when compiling this code. Do you take this detail for granted? > Therefore it's highly likely that this change will make absolutely > no difference whatsoever. I find this outcome unlikely. > (And no it won't improve compile speed in any justifiable way) This was not a goal of my update suggestion for the function "bpf_jit_compile". >> It is generally possible that a specific code generation variant will also affect >> the run time properties you mentioned. > > It's _possible_? Come back with benchmarks. Which code generation variant would be useful to be clarified further? Should we avoid to compare software things similar to "apples" and "oranges" (while these fruits can make more fun)? ;-) > I must also point out that this is a "slow path" i.e. as long as it's > not stupidly inefficient, the speed doesn't matter that much. Can this execution path become warmer (or even "hot") for some other use cases? > This change isn't going to improve the speed of this function by any amount > that matters. This is also possible. > Unless you have some pretty damn good proof that these changes improve things, > there is absolutely no reason to take them as-is Would you care for a better source code structure? > - you are making the code longer Yes. - This is true for the suggested update in this function. > and more difficult to read for no benefit I proposed specific benefits for this software module. > and wasting everyone's time in the process. I assume that a few contributors can take the presented ideas for further considerations. Will their value evolve a bit more later? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On Sun, Sep 4, 2016 at 3:45 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> I hope so. - I propose to give the refactorings "Reduce scope of variable" >>> and "Extract a function" (and the corresponding consequences) another look. >> >> So you _think_ it does. Come back with real proof. > > Which test environments would you find acceptable for further clarification? Compiling it on GCC for Sparc, obviously. >> I must also point out that these sorts of optimisations are things the >> compiler does automatically when compiling this code. > > Do you take this detail for granted? I trust that the GCC developers have done their work well. >> Therefore it's highly likely that this change will make absolutely >> no difference whatsoever. > > I find this outcome unlikely. Then provide proof. >> (And no it won't improve compile speed in any justifiable way) > > This was not a goal of my update suggestion for the function "bpf_jit_compile". I'm looking for some glimmer of usefullness in this patch. I'm not seeing any. >>> It is generally possible that a specific code generation variant will also affect >>> the run time properties you mentioned. >> >> It's _possible_? Come back with benchmarks. > > Which code generation variant would be useful to be clarified further? > > Should we avoid to compare software things similar to "apples" and "oranges" > (while these fruits can make more fun)? ;-) Write a benchmark that exercises this function. Measure the time it took without this change, measure the time it took with this change, is there a difference. You cannot expect people to take you seriously if you're proposing performance changes without any actual ability or interest in producing performance related data to go along with them. >> I must also point out that this is a "slow path" i.e. as long as it's >> not stupidly inefficient, the speed doesn't matter that much. > > Can this execution path become warmer (or even "hot") for some other use cases? That is for you to prove. >> This change isn't going to improve the speed of this function by any amount >> that matters. > > This is also possible. > > >> Unless you have some pretty damn good proof that these changes improve things, >> there is absolutely no reason to take them as-is > > Would you care for a better source code structure? You're essentially saying "I think doing things this way is better" and providing _nothing_ else. I think that things are perfectly fine the way they are. Convince me with data or something else. Was this suggested by some expert? Is this part of some big tree-wide effort to do this that was started by some expert? Did someone do this to some other driver and you're applying the same fix elsewhere? >> - you are making the code longer > > Yes. - This is true for the suggested update in this function. > > >> and more difficult to read for no benefit > > I proposed specific benefits for this software module. What benefits? You have not proved that this change produces any useful benefits. >> and wasting everyone's time in the process. > > I assume that a few contributors can take the presented ideas for further considerations. > Will their value evolve a bit more later? I am subscribed to four fairly high traffic mailing lists and I read hundreds of patches each week. You are the only person proposing changes like these ones as you are (as far as I know) the only person who thinks they have any value. Thanks,
>> Which test environments would you find acceptable for further clarification? > > Compiling it on GCC for Sparc, obviously. Are there any more configuration details to consider? >>> I must also point out that these sorts of optimisations are things the >>> compiler does automatically when compiling this code. >> >> Do you take this detail for granted? > > I trust that the GCC developers have done their work well. Will any more compiler implementations matter here? Do you like software which can run better by default also without application of special compilation parameters? > I'm looking for some glimmer of usefullness in this patch. I'm not seeing any. Thanks for your honest feedback. >> Should we avoid to compare software things similar to "apples" and "oranges" >> (while these fruits can make more fun)? ;-) > > Write a benchmark that exercises this function. Measure the time it > took without this change, measure the time it took with this change, > is there a difference. Is an accepted test system already available for the purpose that every commit would be checked in the way automatically you expect here? > You cannot expect people to take you seriously if you're proposing > performance changes without any actual ability or interest in > producing performance related data to go along with them. I suggested small changes which I found "logical". > You're essentially saying "I think doing things this way is better" Yes … > and providing _nothing_ else. You might be looking for more information than I can practically give you at the moment. > I think that things are perfectly fine the way they are. I have got an other impression for "perfection" in this software module. I found an implementation detail for further considerations. > Convince me with data or something else. I imagine that the "else" can become harder than you find reasonable. > Did someone do this to some other driver and you're applying the same fix elsewhere? Is a similar software development discussion still running for other modules? > You are the only person proposing changes like these ones as you are I am picking special software improvement opportunities up. > (as far as I know) the only person who thinks they have any value. I can accept that the value of specific changes will usually be categorised as lower than updates that you prefer so far. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Markus, I'm really not going to consider any of these changes. And your replies to the feedback you were given disappoint me even more. Please don't submit any more sparc patches until you can get you act in gear and not waste everyone's time. Thank you. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: SF Markus Elfring <elfring@users.sourceforge.net> Date: Sun, 4 Sep 2016 07:45:04 +0200 >> and wasting everyone's time in the process. > > I assume that a few contributors can take the presented ideas for further considerations. > Will their value evolve a bit more later? No, really, you are wasting everyone's time. And as the sole maintainer of the sparc port, you are specifically wasting mine. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, On Sun, Sep 4, 2016 at 4:32 PM, David Miller <davem@davemloft.net> wrote: > > Markus, I'm really not going to consider any of these changes. > > And your replies to the feedback you were given disappoint me even > more. > > Please don't submit any more sparc patches until you can get you act > in gear and not waste everyone's time. In Markus' defence, he does occasionally write okay patches, (for example the kmalloc(a * b) => kmalloc_array(a, b) changes get my small tick of approval) however he also submits a lot of patches which are little more than moving stuff around pointlessly. I really wish he'd concentrate on the former rather than the latter. He's not another Nick Krause. Thanks,
> In Markus' defence, he does occasionally write okay patches, > (for example the kmalloc(a * b) => kmalloc_array(a, b) changes > get my small tick of approval) Thanks for another bit of acceptance for this kind of software change. > however he also submits a lot of patches which are little more than moving stuff around This is true to some degree. > pointlessly. Do we agree to disagree on the value of some collateral software evolutions I'm proposing for a while? > I really wish he'd concentrate on the former rather than the latter. I am on the way for such a software development adventure. There are further improvement opportunities to consider besides the main route, aren't there? > He's not another Nick Krause. Are you going to remember me under an other nickname anyhow? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Markus, On Sun, Sep 4, 2016 at 5:07 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >> I really wish he'd concentrate on the former rather than the latter. > > I am on the way for such a software development adventure. > There are further improvement opportunities to consider besides the main route, > aren't there? You're missing the point here. Find something useful to change. People are going through the kernel finding functions that should be static for example - this is useful as it provides better documentation for those functions (static == not used outside this file) and fixes errors reported by GCC and static code checkers. Those changes are useful. Another useful change would be ensuring that every struct that has a bool member is kzalloc'd instead of kmalloc'd as this eliminates a possible source of undefined behaviour. (A bool is effectively a u8 with behaviour defined for when that value is 0 or 1, kmalloc'd memory can have any data in it, therefore it's possible and likely that a kmalloc'd struct with a bool member will end up with some value for that member that isn't 0 or 1. kzalloc eliminates this possibility.) Moving four assignments because you think they might improve stuff is just annoying people. >> He's not another Nick Krause. > > Are you going to remember me under an other nickname anyhow? Nick Krause submitted patches that make yours look good. At least yours compile. Thanks,
Julian Calaby <julian.calaby@gmail.com> writes: > Otherwise this change is useless churn - you're making the code more > complicated, longer and harder to read for practically no benefit. He has been doing that for years now. And wasted maintainer resources along the way, discussing all the pointless churn. There are zero indications that this will ever change from his side. Do not feed. Bjørn -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c index ced1393..a927470 100644 --- a/arch/sparc/net/bpf_jit_comp.c +++ b/arch/sparc/net/bpf_jit_comp.c @@ -362,10 +362,10 @@ do { *prog++ = BR_OPC | WDISP22(OFF); \ void bpf_jit_compile(struct bpf_prog *fp) { - unsigned int cleanup_addr, proglen, oldproglen = 0; - u32 temp[8], *prog, *func, seen = 0, pass; - const struct sock_filter *filter = fp->insns; - int i, flen = fp->len, pc_ret0 = -1; + unsigned int cleanup_addr, proglen, oldproglen; + u32 temp[8], *prog, *func, seen, pass; + const struct sock_filter *filter; + int i, flen = fp->len, pc_ret0; unsigned int *addrs; void *image; @@ -385,6 +385,10 @@ void bpf_jit_compile(struct bpf_prog *fp) } cleanup_addr = proglen; /* epilogue address */ image = NULL; + filter = fp->insns; + oldproglen = 0; + pc_ret0 = -1; + seen = 0; for (pass = 0; pass < 10; pass++) { u8 seen_or_pass0 = (pass == 0) ? (SEEN_XREG | SEEN_DATAREF | SEEN_MEM) : seen;