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

[Eric Dumazet <edumazet@google.com>]

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.

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.


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