Re: [PATCH 0/2] Replacements for patches 2 and 7 in Big TCP series

[Alexander Duyck <alexander.duyck@gmail.com>]

On Mon, May 9, 2022 at 1:31 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, May 9, 2022 at 1:22 PM Alexander H Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Mon, 2022-05-09 at 11:54 -0700, Eric Dumazet wrote:
> > > On Mon, May 9, 2022 at 11:17 AM Alexander Duyck
> > > <alexander.duyck@gmail.com> wrote:
> > > >
> > > > This patch set is meant to replace patches 2 and 7 in the Big TCP series.
> > > > From what I can tell it looks like they can just be dropped from the series
> > > > and these two patches could be added to the end of the set.
> > > >
> > > > With these patches I have verified that both the loopback and mlx5 drivers
> > > > are able to send and receive IPv6 jumbogram frames when configured with a
> > > > g[sr]o_max_size value larger than 64K.
> > > >
> > > > Note I had to make one minor change to iproute2 to allow submitting a value
> > > > larger than 64K in that I removed a check that was limiting gso_max_size to
> > > > no more than 65536. In the future an alternative might be to fetch the
> > > > IFLA_TSO_MAX_SIZE attribute if it exists and use that, and if not then use
> > > > 65536 as the limit.
> > >
> > > OK, thanks.
> > >
> > > My remarks are :
> > >
> > > 1) Adding these enablers at the end of the series will not be
> > > bisection friendly.
> >
> > They don't have to be added at the end, but essentially they could be
> > drop in replacements for the two patches called out. I just called it
> > out that way as that is what I ended up doing in order to test the
> > patches, and to make it easier to just send them as a pair instead of
> > sending the entire set. I moved them to the end of the list and was
> > swapping between the 2 sets in my testing. I was able to reorder them
> > without any issues. So if you wanted you could place these two patches
> > as patches 2 and 7 in your series.
> >
> > > 2) Lots more changes, and more backport conflicts for us.
> > >
> > > I do not care really, it seems you absolutely hate the new attributes,
> > > I can live with that,
> > > but honestly this makes the BIG TCP patch series quite invasive.
> >
> > As it stands the BIG TCP patch series breaks things since it is
> > outright overrriding the gso_max_size value in the case of IPv6/TCP
> > frames. As I mentioned before this is going to force people to have to
> > update scripts if they are reducing gso_max_size as they would also now
> > need to update gso_ipv6_max_size.
>
> If they never set  gso_ipv6_max_size, they do not have to change it.
> If they set it, well, they get what they wanted.
> Also, the driver value caps  gso_ipv6_max_size, so our patches broke nothing.

I agree that the driver value caps it now that the patches from Jakub
are in. My concern is more with the fact that if they are reducing it
to address some other issue on their NIC then they are now going to
have to update 2 controls instead of just one.

> Some people could actually decide to limit IPV4 TSO packets to 40KB,
> and yet limit
> IPv6 packets to 128KB.
> Their choice.
> Apparently you think this is not a valid choice.

That would be a perfectly valid choice, but limiting it at the NIC
doesn't make sense to me since the NIC is an L2 device and what you
are talking about doing is making modifications up at the L3 layer. It
might make more sense to associate something like that with either a
sysctl at the protocol layer, or maybe even as some sort of attribute
to associate with a routing destination.

I would say the best comparison is device MTU and PMTU or MSS. The
device MTU is a hardware specific value. Nothing larger than that gets
through that specific interface. The PMTU or MSS is what defines your
value from one end to another and is usually stored away in the
routing and/or socket layers. Quite often the PMTU or MSS is smaller
than the device MTU and is tuned in order to get optimal throughput to
the network destination.

> >
> > It makes much more sense to me to allow people to push up the value
> > from 64K to whatever value it is you want to allow for the IPv6/TCP GSO
> > and then just cap the protocols if they cannot support it.
> >
> > As far as the backport/kcompat work it should be pretty straight
> > forward. You just replace the references in the driver to GSO_MAX_SIZE
> > with GSO_LEGACY_MAX_SIZE and then do a check in a header file somewhere
> > via #ifndef and if it doesn't exist you define it.
>
> Well, this is the kind of stuff that Intel loves to do in their
> out-of-tree driver,
> which is kind of horrible.
>
> Look, I will spend fews days rebasing and testing a new series
> including your patches,
> no need to answer this email.
>
> We will live with future merge conflicts, and errors because you
> wanted to change
> GSO_MAX_SIZE, instead of a clean change.

I appreciate all the effort you and the team at Google put into this,
and I am looking forward to seeing it accepted.

Thanks,

- Alex