Message-ID: <20210118105657.72d9a6fe@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 18 Jan 2021 10:56:57 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jonas Bonn <jonas@...rbonn.se>
Cc:     Pravin B Shelar <pbshelar@...com>, netdev@...r.kernel.org,
        pablo@...filter.org, laforge@...monks.org
Subject: Re: [PATCH net-next v5] GTP: add support for flow based tunneling
 API

On Mon, 18 Jan 2021 19:27:53 +0100 Jonas Bonn wrote:
> On 18/01/2021 18:27, Jakub Kicinski wrote:
> > v5 itself was laying around on patchwork for almost a week, marked as
> > "Needs Review/Ack".  
> 
> When new series show up just hours after review, it's hard to take them 
> seriously.  It takes a fair amount of time to go through an elephant 
> like this and to make sense of it; the time spent in response to review 
> commentary shouldn't be less.

Agreed.

> > Normally we try to merge patches within two days. If anything my
> > lesson from this whole ordeal is in fact waiting longer makes
> > absolutely no sense. The review didn't come in anyway, and we're
> > just delaying whatever project Pravin needs this for :/  
> 
> I think the expectation that everything gets review within two days is 
> unrealistic. 

Right, it's perfectly fine to send an email saying "please wait, I'll
review it on $date".

> Worse though, is the insinuation that anything unreviewed 
> gets blindly merged...  No, the two day target should be for the merging 
> of ACK:ed patches.

Well, certainly, the code has to be acceptable to the person merging it.

Let's also remember that Pravin is quite a seasoned contributor.

> > Do I disagree with you that the patch is "far from pretty"? Not at all,
> > but I couldn't find any actual bug, and the experience of contributors
> > matters to us, so we can't wait forever.
> >   
> >> The following issues remain unaddressed after review:
> >>
> >> i)  the patch contains several logically separate changes that would be
> >> better served as smaller patches
> >> ii) functionality like the handling of end markers has been introduced
> >> without further explanation
> >> iii) symmetry between the handling of GTPv0 and GTPv1 has been
> >> unnecessarily broken
> >> iv) there are no available userspace tools to allow for testing this
> >> functionality  
> > 
> > I don't understand these points couldn't be stated on any of the last
> > 3 versions / in the last month.  
> 
> I believe all of the above was stated in review of series v1 and v2.  v3 
> was posted during the merge window so wasn't really relevant for review. 
>   v4 didn't address the comments from v1 and v2.  v5 was posted 3 hours 
> after receiving reverse christmas tree comments and addressed only 
> those.  v5 received commentary within a week... hardly excessive for a 
> lightly maintained module like this one.

Sorry, a week is far too long for netdev. If we were to wait that long
we'd have a queue of 300+ patches always hanging around.

> >> I have requested that this patch be reworked into a series of smaller
> >> changes.  That would allow:
> >>
> >> i) reasonable review
> >> ii) the possibility to explain _why_ things are being done in the patch
> >> comment where this isn't obvious (like the handling of end markers)
> >> iii) the chance to do a reasonable rebase of other ongoing work onto
> >> this patch (series):  this one patch is invasive and difficult to rebase
> >> onto
> >>
> >> I'm not sure what the hurry is to get this patch into mainline.  Large
> >> and complicated patches like this take time to review; please revert
> >> this and allow that process to happen.  
> > 
> > You'd need to post a revert with the justification to the ML, so it can
> > be reviewed on its merits. That said I think incremental changes may be
> > a better direction.
> 
> I guess I'll have to do so, but that seems like setting the bar higher 
> than for even getting the patch in in the first place.
> 
> I don't think it's tenable for patches to sneak in because they are so 
> convoluted that the maintainers just can't find the energy to review 
> them.  I'd say that the maintainers silence on this particular patch 
> speaks volumes in itself.

Sadly most maintainers are not particularly dependable, so we can't
afford to make that the criteria.

I have also pinged for reviews on v4 and nobody replied.

> Sincerely frustrated because rebasing my IPv6 series on top of this mess 
> will take days,

I sympathize, perhaps we should document the expectations we have so
less involved maintainers know the expectations :(

Powered by blists - more mailing lists