On Sun, Jan 17, 2021 at 5:25 AM Jonas Bonn <jo...@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 <pbshe...@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.
