Re: [PATCH net-next v5] GTP: add support for flow based tunneling API

[Jakub Kicinski <kuba@kernel.org>]

On Sun, 17 Jan 2021 14:23:52 +0100 Jonas Bonn wrote:
> On 17/01/2021 01:46, Jakub Kicinski wrote:
> > On Sat,  9 Jan 2021 23:00:21 -0800 Pravin B Shelar wrote:  
> >> Following patch add support for flow based tunneling API
> >> to send and recv GTP tunnel packet over tunnel metadata API.
> >> This would allow this device integration with OVS or eBPF using
> >> flow based tunneling APIs.
> >>
> >> Signed-off-by: Pravin B Shelar <pbshelar@fb.com>  
> > 
> > Applied, thanks!
> 
> This patch hasn't received any ACK's from either the maintainers or 
> anyone else providing review.  

I made Pravin wait _over_ _a_ _month_ to merge this. He did not receive
any feedback since v3, which was posted Dec 13th. That's very long.

v5 itself was laying around on patchwork for almost a week, marked as 
"Needs Review/Ack".

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 :/

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 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.