Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Nov 2022 08:59:06 +0100
From:   'Horatiu Vultur' <horatiu.vultur@microchip.com>
To:     David Laight <David.Laight@aculab.com>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net v2 0/3] net: lan966x: Fixes for when MTU is changed
Message-ID: <20221101075906.ets6zsti3c54idae@soft-dev3-1>
References: <20221030213636.1031408-1-horatiu.vultur@microchip.com>
 <b75a7136030846f587e555763ef2750e@AcuMS.aculab.com>
 <20221031150133.2be5xr7cmuhr4gng@soft-dev3-1>
 <219ebe83a5ad4467937545ee5a0e77e4@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <219ebe83a5ad4467937545ee5a0e77e4@AcuMS.aculab.com>
Precedence: bulk

The 10/31/2022 15:27, David Laight wrote:
> 
> From: 'Horatiu Vultur'
> > Sent: 31 October 2022 15:02
> >
> > The 10/31/2022 10:43, David Laight wrote:
> > >
> > > From: Horatiu Vultur
> > > > Sent: 30 October 2022 21:37
> >
> > Hi David,
> >
> > > >
> > > > There were multiple problems in different parts of the driver when
> > > > the MTU was changed.
> > > > The first problem was that the HW was missing to configure the correct
> > > > value, it was missing ETH_HLEN and ETH_FCS_LEN. The second problem was
> > > > when vlan filtering was enabled/disabled, the MRU was not adjusted
> > > > corretly. While the last issue was that the FDMA was calculated wrongly
> > > > the correct maximum MTU.
> > >
> > > IIRC all these lengths are 1514, 1518 and maybe 1522?
> >
> > And also 1526, if the frame has 2 vlan tags.
> >
> > > How long are the actual receive buffers?
> > > I'd guess they have to be rounded up to a whole number of cache lines
> > > (especially on non-coherent systems) so are probably 1536 bytes.
> >
> > The receive buffers can be different sizes, it can be up to 65k.
> > They are currently allign to page size.
> 
> Is that necessary?

HW requires to have the start of frame alligned to 128 bytes.

> I don't know where the buffers are allocated, but even 4k seems
> a bit profligate for normal ethernet mtu.
> If the page size if larger it is even sillier.

For lan966x the pages are allocated here [1]

> 
> If the buffer is embedded in an skb you really want the skb
> to be under 4k (I don't think a 1500 byte mtu can fit in 2k).
> 
> But you might as well tell the hardware the actual buffer length
> (remember to allow for the crc and any alignment header).

I am already doing that here [2]
And I need to do it for each frame it can received.

> 
> > >
> > > If driver does support 8k+ jumbo frames just set the hardware
> > > frame length to match the receive buffer size.
> >
> > In that case I should always allocate maximum frame size(65k) for all
> > regardless of the MTU?
> 
> That would be very wasteful.

Yes, I agree.

> I'd set the buffer large enough for the mtu but let the hardware fill
> the entire buffer.

I am not 100% sure I follow it. Can you expend on this a little bit?

> 
> Allocating 64k buffers for big jumbo frames doesn't seem right.
> If the mtu is 64k then kmalloc() will allocate 128k.
> This is going to cause 'oddities' with small packets where
> the 'true_size' is massively more than the data size.
> 
> Isn't there a scheme where you can create an skb from a page
> list that contains fragments of the ethernet frame?

Can I use '__skb_fill_page_desc'?

> In which case I'd have thought you'd want to fill the ring
> with page size buffers and then handle the hardware writing
> a long frame to multiple buffers/descriptors.

It might be a good idea. I need to look in more details about this.
Because it would change a little bit the logic on how the frames are
received and see how this will impact for frames under a page.
Also I was thinking next to use page_pool API, for which I send a patch
[3] but is deffered by this patch series.
But all these possible changes will need to go through net-next.

> 
>         David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

[1] https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c#L17
[2] https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c#L70
[3] https://lore.kernel.org/bpf/20221019135008.3281743-6-horatiu.vultur@microchip.com/

-- 
/Horatiu