Re: [EXT] [PATCH] aquantia: Reserve space when allocating an SKB

["Ramsay, Lincoln" <Lincoln.Ramsay@digi.com>]

Hi Igor,

> Here I understand your intention. You are trying to "offset" the placement of
> the packet data, and the restore it back when construction SKB.

Originally, I just added the skb_reserve call, but that broke everything. When I looked at what the igb driver was doing, this approach seemed reasonable but I wasn't sure it'd work.

> The problem however is that hardware is being programmed with fixed descriptor
> size for placement. And its equal to AQ_CFG_RX_FRAME_MAX (2K by default).
> 
> This means, HW will do writes of up to 2K packet data into a single
> descriptor, and then (if not enough), will go for next descriptor data.
> 
> With your solution, packets of size (AQ_CFG_RX_FRAME_MAX - AQ_SKB_PAD) up to
> size of AQ_CFG_RX_FRAME_MAX will overwrite the area of page they designated
> to. Ultimately, HW will do a memory corruption of next page.

Yeah... this is the kind of thing I was worried about. It seemed to me that the SKB was being built around a hardware buffer rather than around heap-allocated memory. I just hoped that the rx_off value would somehow make it work.

The code in aq_get_rxpages seems to suggest that multiple frames can fit in a rxpage, so maybe the logic there prevents overwriting? (at the expense of not fitting as many frames into the page before it has to get a new one?)

I didn't notice any issues when I was testing, but apart from port forwarding ssh (which is tiny) and some copying of files on (probably not even close to saturating the link) there's not a huge network load placed on the device. I guess it's entirely possible that an overwrite problem would only show up under heavy load? (ie. more, and larger amounts of data in flight through the kernel at once)

> I think the only acceptable solution here would be removing that optimized
> path of build_skb, and keep only napi_alloc_skb. Or, we can think of keeping
> it under some configuration condition (which is also not good).

I'll attempt to confirm that this works too, at least for our tests :)

Lincoln