archive mirror
 help / color / mirror / Atom feed
From: "Ramsay, Lincoln" <>
To: Igor Russkikh <>,
	"David S. Miller" <>,
	Jakub Kicinski <>,
	"" <>,
	Dmitry Bogdanov <>
Subject: Re: [EXT] [PATCH] aquantia: Reserve space when allocating an SKB
Date: Thu, 19 Nov 2020 00:14:05 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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 :)


  reply	other threads:[~2020-11-19  0:14 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-18  1:52 [PATCH] aquantia: Reserve space when allocating an SKB Ramsay, Lincoln
2020-11-18 14:02 ` [EXT] " Igor Russkikh
2020-11-19  0:14   ` Ramsay, Lincoln [this message]
2020-11-19  5:19     ` Ramsay, Lincoln
2020-11-19 22:01       ` [PATCH] aquantia: Remove the build_skb path Ramsay, Lincoln
2020-11-19 22:07         ` [PATCH v2] " Ramsay, Lincoln
2020-11-19 22:15           ` Florian Westphal
2020-11-19 22:24             ` Ramsay, Lincoln
2020-11-19 22:28               ` Florian Westphal
2020-11-19 22:34                 ` [PATCH v3] " Ramsay, Lincoln
2020-11-19 22:49                   ` Maciej Fijalkowski
2020-11-20  8:18                     ` [EXT] " Igor Russkikh
2020-11-23 19:28                       ` Maciej Fijalkowski
2020-11-24 15:26                         ` Igor Russkikh
2020-11-19 22:58                   ` Florian Westphal
2020-11-19 23:52                     ` [PATCH v4] " Ramsay, Lincoln
2020-11-20  0:17                       ` Florian Westphal
2020-11-20  0:23                         ` Ramsay, Lincoln
2020-11-21 21:22                       ` Jakub Kicinski
2020-11-21 21:23                         ` Jakub Kicinski
2020-11-22 22:36                           ` Ramsay, Lincoln
2020-11-23 16:42                             ` Jakub Kicinski
2020-11-23 21:40                               ` [PATCH net v5] " Ramsay, Lincoln
2020-11-24 19:02                                 ` Jakub Kicinski
2020-11-22 21:55                         ` [PATCH v4] " Ramsay, Lincoln
2020-11-20  7:52         ` [EXT] [PATCH] " Igor Russkikh
2020-11-23  4:20           ` Ramsay, Lincoln
2020-11-24 14:29             ` Igor Russkikh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).