| Message ID | Pine.LNX.4.64.0902070040530.31875@wrl-59.cs.helsinki.fi |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> Date: Sat, 7 Feb 2009 00:58:44 +0200 (EET) > I think your "English" is too strict requirement. I absolutely and whole-heartedly agree. This set of requirements is beyond rediculious and will do nothing but deter people from contributing. So unless that's what the goal is... Anyways, I'll be applying Ilpo's great changes to the net-next-2.6 GIT tree, without putting him through the ringer like this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2009-02-06 at 19:22 -0800, David Miller wrote: > From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> > Date: Sat, 7 Feb 2009 00:58:44 +0200 (EET) > > > I think your "English" is too strict requirement. > > I absolutely and whole-heartedly agree. This set of > requirements is beyond rediculious and will do nothing > but deter people from contributing. > > So unless that's what the goal is... > > Anyways, I'll be applying Ilpo's great changes to the > net-next-2.6 GIT tree, without putting him through the > ringer like this. No. I find it quite unacceptable for the networking group to circumvent the standard Linux review process in this manner. If you plan to push a patch into the sunrpc code against the wishes of the community and people that maintain that code then I'll be petitioning Linus to revert it on general principle as soon as it hits his tree. Like it or not, the changelog _is_ a part of the patch that is subject to review and comment, and while I like the rest of the patch, I will support Chuck and Bruce's comments that putting a bunch of diffstat output into the changelog is not a substitute for a simple sentence. All it takes is " Save 147 lines by moving the common code from xs_udp_write_space() and xs_tcp_write_space() into a separate helper. S-o-b: Ilpo.... " and everyone will be happy. So please stop making a storm in a teacup out of this... Trond
On Sat, 7 Feb 2009, Trond Myklebust wrote: > On Fri, 2009-02-06 at 19:22 -0800, David Miller wrote: > > From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> > > Date: Sat, 7 Feb 2009 00:58:44 +0200 (EET) > > > > > I think your "English" is too strict requirement. > > > > I absolutely and whole-heartedly agree. This set of > > requirements is beyond rediculious and will do nothing > > but deter people from contributing. > > > > So unless that's what the goal is... > > > > Anyways, I'll be applying Ilpo's great changes to the > > net-next-2.6 GIT tree, without putting him through the > > ringer like this. > > No. I find it quite unacceptable for the networking group to circumvent > the standard Linux review process in this manner. If you plan to push a > patch into the sunrpc code against the wishes of the community and > people that maintain that code then I'll be petitioning Linus to revert > it on general principle as soon as it hits his tree. :-) > Like it or not, the changelog _is_ a part of the patch that is subject > to review and comment, and while I like the rest of the patch, I will > support Chuck and Bruce's comments that putting a bunch of diffstat > output into the changelog is not a substitute for a simple sentence. But this is likely based on a false assumption (see my thought about that below), and I didn't have diffstat afaik :-). > All it takes is > > " > Save 147 lines by moving the common code from xs_udp_write_space() and > xs_tcp_write_space() into a separate helper. > > S-o-b: Ilpo.... > " Can you please tell if you're still under impression that there wasn't any changelog? What about those other guys who raised their concerns? (I can certainly understand why this impression might develop but it's a problem in the canonial patch format, not in my actual changelog, I even need to circumvent ^--- by adding a space the to prevent tools for making a mistake). It will make a lot more sense after getting applied. ...Because there certainly was a changelog, and I didn't have any diffstat output there in the changelog?!? The output that *is* in the changelog is supposed to *help* the reviewer to see what will be still necessary to keep in the separate functions, but I guess people missed that because the changelog looks too similar to what you will categorize as patch (diff-funcs uses output which is familiar to all who who deal with patches, and that's for purpose!). > and everyone will be happy. So please stop making a storm in a teacup > out of this... I even tried to make sunrpc people happy by sending the followup with some plain English in changelog too (though it seems very obvious as you could spell it out above without a question besides the s/bytes/lines/ :-)) and even more clear title. I agree that I should have put, "; add helper" or something like that to the title which is exactly what I did in the followup. I personally think that people were honestly just mislead to think that diff-funcs output is part of the patch -> thus it raises complaining about missing changelog (which is not true). But whatever :-). Feel free to revert the first one and put the followup which has something to satisfy all :-).
On Sat, 2009-02-07 at 18:56 +0200, Ilpo Järvinen wrote: > Can you please tell if you're still under impression that there wasn't > any changelog? What about those other guys who raised their concerns? > (I can certainly understand why this impression might develop but it's a > problem in the canonial patch format, not in my actual changelog, I even > need to circumvent ^--- by adding a space the to prevent tools for making > a mistake). It will make a lot more sense after getting applied. > ...Because there certainly was a changelog, and I didn't have any > diffstat output there in the changelog?!? The output that *is* in the > changelog is supposed to *help* the reviewer to see what will be still > necessary to keep in the separate functions, but I guess people missed > that because the changelog looks too similar to what you will categorize > as patch (diff-funcs uses output which is familiar to all who who deal > with patches, and that's for purpose!). If Chuck and Bruce are happy with it, then there is no problem, however I have yet to see an acked-by from either one of them. My point is that the process for accepting patches into the kernel involves ensuring that there is consensus. It does not allow for one group to ride roughshod over the objections of another. Trond
From: Trond Myklebust <Trond.Myklebust@netapp.com> Date: Sat, 07 Feb 2009 12:22:01 -0500 > My point is that the process for accepting patches into the kernel > involves ensuring that there is consensus. You haven't been hacking the kernel very long if you actually believe this. When people are being rediculious or unreasonable, subsystem or higher maintainers make exective decisions all the time. "revert based upon principle" ?!?! If you think Linus will do that for a correct change, you're kidding yourself. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote: > I personally think that people were honestly just mislead to think that > diff-funcs output is part of the patch Yes, exactly, in my case, that's all it was--the "diff" I thought I saw in my 2-second skim looked odd, and I figured a comment might have gotten dropped off in the process of replying and adding cc's--hence the original question. Anyway, the patch looks obviously fine to me, and the diff-func and codiff output are helpful. (Where's diff-func from?) In similar cases in the future, if you want to cater to the more tired and/or mentally challenged among us (which may just be me), some clue that visually distinguishes the quoted diff output in the comment might help; e.g., maybe indent it like the below. (But that's intended as a suggestion, not a requirement.) --b. Subject: Re: [PATCH] net/sunrpc/xprtsock.c: some common code found diff-func output suggests these functions are very similar: $ diff-funcs xs_udp_write_space net/sunrpc/xprtsock.c ...(etc) and codiff shows the space savings: $ codiff net/sunrpc/xprtsock.o net/sunrpc/xprtsock.o.new ...(etc) Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote: > On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote: >> I personally think that people were honestly just mislead to think >> that >> diff-funcs output is part of the patch > > Yes, exactly, in my case, that's all it was--the "diff" I thought I > saw > in my 2-second skim looked odd, and I figured a comment might have > gotten dropped off in the process of replying and adding cc's--hence > the > original question. I apologize if my request for "English" seemed Anglo-centric. What I intended was "non-machine-generated" and expressed in the natural language that is usual for Linux kernel patch descriptions, which appears to be English. Not being a maintainer myself, I can't enforce any requirement here. I was simply suggesting ways to help the patch make its way expediently to reviewers and people who can get it into the kernel. I agree with you that the diff output in the patch description was confusing, and yes, after Dave's first comment I understood what the description meant. I believe patch authors owe it to maintainers, who have dozens of patches to review a day, to keep it as simple and easy as possible. I have tried to work with individual maintainers where possible to improve the legibility of my patches, though at times I have fallen short. Although interesting, the tool output doesn't seem necessary to justify the proposed change. A single sentence, as Trond suggested, would have been far more concise. And Bruce's recommendations for setting off machine-generated content in a patch description are cogent. In addition to crafting a legible description, my suggestions were about cc'ing the proper mailing list, and routing this patch directly to the sunrpc kernel maintainers in the first place, all of which are listed within easy reach in MAINTAINERS. A handful of us on linux-nfs happen to read netdev, but not all of us. We do try to stick to a fair review process here, which includes archiving the patch and reviewer comments via linux-nfs@vger.kernel.org. J. Bruce Fields continues: > Anyway, the patch looks obviously fine to me[.] I don't have an immediate problem with the patch's content either, as long as it has been tested and does not introduce any new sparse or compiler warnings. I'm all for the intelligent elimination of gotos and duplicated code. But I wonder how we ended up with these two copies of the same logic. It may be that at some point in the past we had more different logic in the two write_space functions, but I suppose we can split it out again if we ever need to. Thinking aloud, at some point we may have lost a little history here, or we may have perhaps lost any implicit operational dependencies on other parts of xprtsock.c (like the TCP state logic) that are not expressed in the source code or comments. The newly shared code appears to be socket-specific, so it does not belong in the shared function we already have, xprt_write_space(), which is meant to be transport-generic. Acked-by: Chuck Lever <chuck.lever@oracle.com> -- Chuck Lever chuck[dot]lever[at]oracle[dot]com-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 9 Feb 2009, Chuck Lever wrote: > On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote: > >On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote: > > >I personally think that people were honestly just mislead to think that > > >diff-funcs output is part of the patch > > > >Yes, exactly, in my case, that's all it was--the "diff" I thought I saw > >in my 2-second skim looked odd, and I figured a comment might have > >gotten dropped off in the process of replying and adding cc's--hence the > >original question. > > I apologize if my request for "English" seemed Anglo-centric. What I intended > was "non-machine-generated" and expressed in the natural language that is > usual for Linux kernel patch descriptions, which appears to be English. ...I still argue that if title gives the same information as that sentence would, the sentence is optional. Some patches don't even have patch description beyond title and there's very little problem with that. As I admitted earlier the title wasn't as good as it should have (and I even sent a followup to please all including even the in body sentence plus the fixed title). Not that I find the title really a problem to sane people who understand that code-duplication is bad and getting rid of it is what one should do in general, as long as it's possible without making a mess out of it. > I agree with you that the diff output in the patch description was confusing, > and yes, after Dave's first comment I understood what the description meant. > I believe patch authors owe it to maintainers, who have dozens of patches to > review a day, to keep it as simple and easy as possible. I have tried to work > with individual maintainers where possible to improve the legibility of my > patches, though at times I have fallen short. > > Although interesting, the tool output doesn't seem necessary to justify the > proposed change. I don't understand why a tool output cannot "say" the same as some English sentence... And if a proof that the code blocks that were merged were infact identical (instead of having some hard to spot difference) doesn't seem like useful information to reviewer I don't really know what to say. > In addition to crafting a legible description, my suggestions were about > cc'ing the proper mailing list, and routing this patch directly to the sunrpc > kernel maintainers in the first place, all of which are listed within easy > reach in MAINTAINERS. Please note that I couldn't guess that I'd have to look for another entry after first SUNRPC entry in the MAINTAINERS. So asking outsider to know who is who in sunrpc realm is not very likely to be a practical requirement either. And even to add into confusion the latest commits in that particular directory had the last S-o-B line as Bruce's. How could I have guessed? Forgetting to lookup the address from MAINTAINERS into the first mail is something which just happened and I'm certainly guilty to that :-). ...I've had enough down turning experiences with random dev lists listed in the MAINTAINERS (mostly spammy subsriber only ones, nearly infinite response time, etc things) that I mostly ignore them nowadays though this time it would have been vger list so no subscription required madness. Instead I either select lkml or netdev, this time netdev because of the obvious presence under net/ hierarcy. > But I wonder how we ended up with these two copies of the same logic. It may > be that at some point in the past we had more different logic in the two > write_space functions, but I suppose we can split it out again if we ever need > to. Thinking aloud, at some point we may have lost a little history here, or > we may have perhaps lost any implicit operational dependencies on other parts > of xprtsock.c (like the TCP state logic) that are not expressed in the source > code or comments. Well, this kind of thinking is beyond me and requires more knowledge about internals of the code. IMHO, this kind of speculation changes nothing really, and besides the functions _were_ different to begin with as was proven in the patch description :-). ...Would they have been fully identical you would have seen a bit different patch ;-). If you think there's something wrong, please fix, you're free to do so, I don't understand enough. But we don't keep all those #if 0 blocks around either just because somebody thinks that something will get implemented one day (not that you directly made such assertion, probably saying mildy the opposite instead). Ironically, you would have been perfectly happy with the particular functions unless a cleanup patch against them gets made... :-) > The newly shared code appears to be socket-specific, so it does not > belong in the shared function we already have, xprt_write_space(), which > is meant to be transport-generic. IMHO, this is the most useful response so far! :-) Usually the hardest obstacles I face with non-familiar code is its structure and naming.
On Sun, 8 Feb 2009, J. Bruce Fields wrote: > (Where's diff-func from?) My own tool collection. I can make it available if you want to. > In similar cases in the future, if you want to cater to the more tired > and/or mentally challenged among us (which may just be me), some clue > that visually distinguishes the quoted diff output in the comment might > help; e.g., maybe indent it like the below. > > (But that's intended as a suggestion, not a requirement.) Yeah, a good idea... I'll sed some spaces in front of them in the future to make the difference more obvious. The expected outcome though is that somebody probably then hits reply button too soon claiming that the patch is broken :-).
Hi Ilpo- On Feb 10, 2009, at 8:12 AM, Ilpo Järvinen wrote: > On Mon, 9 Feb 2009, Chuck Lever wrote: > >> On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote: >>> On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote: >>>> I personally think that people were honestly just mislead to >>>> think that >>>> diff-funcs output is part of the patch >>> >>> Yes, exactly, in my case, that's all it was--the "diff" I thought >>> I saw >>> in my 2-second skim looked odd, and I figured a comment might have >>> gotten dropped off in the process of replying and adding cc's-- >>> hence the >>> original question. >> >> I apologize if my request for "English" seemed Anglo-centric. What >> I intended >> was "non-machine-generated" and expressed in the natural language >> that is >> usual for Linux kernel patch descriptions, which appears to be >> English. > > ...I still argue that if title gives the same information as that > sentence would, the sentence is optional. I'm OK with that. In the original patch, I think both were somewhat difficult to parse. My request is to try harder next time to make them clear. It sounds like we agree here. > Some patches don't even have > patch description beyond title and there's very little problem with > that. > As I admitted earlier the title wasn't as good as it should have > (and I > even sent a followup to please all including even the in body sentence > plus the fixed title). Not that I find the title really a problem to > sane > people who understand that code-duplication is bad and getting rid > of it > is what one should do in general, as long as it's possible without > making > a mess out of it. Like all things software, absolute statements are usually always proven incorrect (eg "goto is spawn of the devil and should never be used"). Code duplication is _usually_ bad. Sometimes there is a reason for it. We want to avoid a knee-jerk response of eliminating it wherever we find it, without another thought. >> I agree with you that the diff output in the patch description was >> confusing, >> and yes, after Dave's first comment I understood what the >> description meant. >> I believe patch authors owe it to maintainers, who have dozens of >> patches to >> review a day, to keep it as simple and easy as possible. I have >> tried to work >> with individual maintainers where possible to improve the >> legibility of my >> patches, though at times I have fallen short. >> >> Although interesting, the tool output doesn't seem necessary to >> justify the >> proposed change. > > I don't understand why a tool output cannot "say" the same as some > English > sentence... It can. But it's interesting to observe that the point of your proposed change was to reduce lines of code and code obscurity. Your description, however, was much longer and more obscure than it needed to be. :-) I was too harsh to ask for no machine-generated patch descriptions. The point is, is the description meaningful and concise? > And if a proof that the code blocks that were merged were > infact identical (instead of having some hard to spot difference) > doesn't > seem like useful information to reviewer I don't really know what to > say. Verification of your patch is different than justification. But that's a minor point. >> In addition to crafting a legible description, my suggestions were >> about >> cc'ing the proper mailing list, and routing this patch directly to >> the sunrpc >> kernel maintainers in the first place, all of which are listed >> within easy >> reach in MAINTAINERS. > > Please note that I couldn't guess that I'd have to look for another > entry > after first SUNRPC entry in the MAINTAINERS. So asking outsider to > know > who is who in sunrpc realm is not very likely to be a practical > requirement either. And even to add into confusion the latest commits > in that particular directory had the last S-o-B line as Bruce's. How > could > I have guessed? Forgetting to lookup the address from MAINTAINERS > into the > first mail is something which just happened and I'm certainly guilty > to > that :-). Yes, we should fix the SUNRPC entries in MAINTAINERS. > ...I've had enough down turning experiences with random dev lists > listed > in the MAINTAINERS (mostly spammy subsriber only ones, nearly infinite > response time, etc things) that I mostly ignore them nowadays though > this > time it would have been vger list so no subscription required madness. > Instead I either select lkml or netdev, this time netdev because of > the obvious presence under net/ hierarcy. My experience is usually the opposite. I post a patch directly to a maintainer and get a "please use such-and-such list" response. The exception is usually very specific device drivers. But you're right, it would be nice to have some consistency here across the kernel. >> But I wonder how we ended up with these two copies of the same >> logic. It may >> be that at some point in the past we had more different logic in >> the two >> write_space functions, but I suppose we can split it out again if >> we ever need >> to. Thinking aloud, at some point we may have lost a little >> history here, or >> we may have perhaps lost any implicit operational dependencies on >> other parts >> of xprtsock.c (like the TCP state logic) that are not expressed in >> the source >> code or comments. > > Well, this kind of thinking is beyond me and requires more knowledge > about > internals of the code. My concern with immediately accepting a patch that appears to be solely machine-generated is exactly this. The submission didn't leave me with the impression that there was a human who had taken the time to understand history (git log/annotate) and how the code works, what the source code was attempting to express, or whether the resulting patch was even tested. Usually you can glean a bit of this from a lengthier patch description. Yes, yours was a simple and proper change in the end, but still. Given the long "dialog" just had on lkml about compiler (machine- generated) optimizations, I think we would all be just a little skeptical (or perhaps careful) about such automated transformations. > IMHO, this kind of speculation changes nothing > really, and besides the functions _were_ different to begin with as > was > proven in the patch description :-). ...Would they have been fully > identical you would have seen a bit different patch ;-). > > If you think there's something wrong, please fix, you're free to do > so, > I don't understand enough. But we don't keep all those #if 0 blocks > around either just because somebody thinks that something will get > implemented one day (not that you directly made such assertion, > probably > saying mildy the opposite instead). Ironically, you would have been > perfectly happy with the particular functions unless a cleanup patch > against them gets made... :-) You are correct, the analysis doesn't change anything, but the analysis is worth at least noting during review. It helps us understand where we have to make the code more clear. The question is whether the code itself documents dependencies on other parts of the code, whether such dependencies are only documented by comments, or whether it is entirely implicit and up to the folks who work on it to understand and remember. This is usually only an issue when someone who has never worked on the code submits a patch. The goal of open source code is to facilitate other folks modifying our code. For open source especially it is a larger issue than simple code maintenance. It is not enough for open source code simply to work. If people outside our team (or outside the larger Linux community) can't easily understand the important features of our open source code, then we've still failed, even if the legal barriers have been lifted by the GPL. Thus I'm especially sensitive to areas where our source code does not express itself well to folks who are not familiar. We want to invite wider review, and that's simply not possible if nobody understands this code base but ourselves. >> The newly shared code appears to be socket-specific, so it does not >> belong in the shared function we already have, xprt_write_space(), >> which >> is meant to be transport-generic. > > IMHO, this is the most useful response so far! :-) Usually the hardest > obstacles I face with non-familiar code is its structure and naming. -- Chuck Lever chuck[dot]lever[at]oracle[dot]com-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 10, 2009 at 03:20:36PM +0200, Ilpo Järvinen wrote: > On Sun, 8 Feb 2009, J. Bruce Fields wrote: > > > (Where's diff-func from?) > > My own tool collection. I can make it available if you want to. Sure, I'd be curious. --b. > > > In similar cases in the future, if you want to cater to the more tired > > and/or mentally challenged among us (which may just be me), some clue > > that visually distinguishes the quoted diff output in the comment might > > help; e.g., maybe indent it like the below. > > > > (But that's intended as a suggestion, not a requirement.) > > Yeah, a good idea... I'll sed some spaces in front of them in the future > to make the difference more obvious. The expected outcome though is that > somebody probably then hits reply button too soon claiming that the patch > is broken :-). > > > -- > i. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c index 5cbb404..b49e434 100644 --- a/net/sunrpc/xprtsock.c +++ b/net/sunrpc/xprtsock.c @@ -1215,6 +1215,23 @@ out: read_unlock(&sk->sk_callback_lock); } +static void xs_write_space(struct sock *sk) +{ + struct socket *sock; + struct rpc_xprt *xprt; + + if (unlikely(!(sock = sk->sk_socket))) + return; + clear_bit(SOCK_NOSPACE, &sock->flags); + + if (unlikely(!(xprt = xprt_from_sock(sk)))) + return; + if (test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags) == 0) + return; + + xprt_write_space(xprt); +} + /** * xs_udp_write_space - callback invoked when socket buffer space * becomes available @@ -1230,23 +1247,9 @@ static void xs_udp_write_space(struct sock *sk) read_lock(&sk->sk_callback_lock); /* from net/core/sock.c:sock_def_write_space */ - if (sock_writeable(sk)) { - struct socket *sock; - struct rpc_xprt *xprt; - - if (unlikely(!(sock = sk->sk_socket))) - goto out; - clear_bit(SOCK_NOSPACE, &sock->flags); - - if (unlikely(!(xprt = xprt_from_sock(sk)))) - goto out; - if (test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags) == 0) - goto out; - - xprt_write_space(xprt); - } + if (sock_writeable(sk)) + xs_write_space(sk); - out: read_unlock(&sk->sk_callback_lock); } @@ -1265,23 +1268,9 @@ static void xs_tcp_write_space(struct sock *sk) read_lock(&sk->sk_callback_lock); /* from net/core/stream.c:sk_stream_write_space */ - if (sk_stream_wspace(sk) >= sk_stream_min_wspace(sk)) { - struct socket *sock; - struct rpc_xprt *xprt; - - if (unlikely(!(sock = sk->sk_socket))) - goto out; - clear_bit(SOCK_NOSPACE, &sock->flags); + if (sk_stream_wspace(sk) >= sk_stream_min_wspace(sk)) + xs_write_space(sk); - if (unlikely(!(xprt = xprt_from_sock(sk)))) - goto out; - if (test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sock->flags) == 0) - goto out; - - xprt_write_space(xprt); - } - - out: read_unlock(&sk->sk_callback_lock); }