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

From: Pravin Shelar <pravin.ovn@gmail.com>
To: Jonas Bonn <jonas@norrbonn.se>
Cc: Jakub Kicinski <kuba@kernel.org>,
	Pravin B Shelar <pbshelar@fb.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	Harald Welte <laforge@gnumonks.org>
Subject: Re: [PATCH net-next v5] GTP: add support for flow based tunneling API
Date: Sun, 17 Jan 2021 12:47:30 -0800	[thread overview]
Message-ID: <CAOrHB_CvFV1K2J_v1L50Q=zhiTVH3maq4tyzskVmyP-di-wXtw@mail.gmail.com> (raw)
In-Reply-To: <8adc4450-c32d-625e-3c8c-70dbd7cbf052@norrbonn.se>

On Sun, Jan 17, 2021 at 5:25 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi Jakub,
>
> 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.  The following issues remain unaddressed
> after review:
>
This patch was first sent out on Dec 10 and you responded on Dec 11. I
think there was a reasonable amount of time given for reviews.

> i)  the patch contains several logically separate changes that would be
> better served as smaller patches
Given this patch is adding a single feature, to add support for LWT, I
sent a single patch. Now sure how much benefit it would be to divide
it in smaller patches. In future I will be more careful with dividing
the patch, since that is THE objection you have presented here.

> ii) functionality like the handling of end markers has been introduced
> without further explanation
This is the first time you are raising this question. End marker is
introduced to handle these packets in a single LWT device. This way
the control plane can use a single device to program all GTP
user-plane functionality.

> iii) symmetry between the handling of GTPv0 and GTPv1 has been
> unnecessarily broken
This is already discussed in previous review: Once we add support for
LWT for v0, we can get symmetry between V1 and V0. At this point there
is no use case to use V0 in LWT, so I do not see a point introducing
the support.

> iv) there are no available userspace tools to allow for testing this
> functionality
>
This is not true. I have mentioned and provided open source tools
multiple times on review tread:

Patch for iproute to add support for LWT GTP devices.
https://github.com/pshelar/iproute2/commit/d6e99f8342672e6e9ce0b71e153296f8e2b41cfc

OVS support with integration test:
https://github.com/pshelar/ovs/blob/6ec6a2a86adc56c7c9dcab7b3a7b70bb6dad35c9/tests/system-layer3-tunnels.at#L158

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

Rather than reverting this patch, I can handle comments you have
posted. Those can be fixed by minor incremental patches.

Let me know if you find any regression introduced by this patch set, I
can quickly fix it.

Thanks,
Pravin.