From: "Ramsay, Lincoln" <Lincoln.Ramsay@digi.com>
To: Igor Russkikh <irusskikh@marvell.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Dmitry Bogdanov <dbogdanov@marvell.com>
Subject: Re: [EXT] [PATCH] aquantia: Reserve space when allocating an SKB
Date: Thu, 19 Nov 2020 05:19:56 +0000 [thread overview]
Message-ID: <CY4PR1001MB231125B16A35324A79270373E8E00@CY4PR1001MB2311.namprd10.prod.outlook.com> (raw)
In-Reply-To: <CY4PR1001MB2311C0DA2840AFC20AE6AEB5E8E10@CY4PR1001MB2311.namprd10.prod.outlook.com>
Hi Igor,
> > 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.
>
> 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 am not terribly experienced with such low-level code, but it looks to me looks like a page is allocated (4k) and then DMA mapped to the device. Frames are 2k, so only 2 can fit into a single mapped page. If the mapping was done with an offset of AQ_SKB_PAD, that'd leave space for the SKB headroom but it would mean only a single frame could fit into that mapped page. Since this is the "I only have 1 fragment less than 2k" code path, maybe that's ok? I'm not sure if the hardware side can know that it's only allowed to write 1 frame into the buffer...
I noticed on my device that aq_ring_rx_clean always hits the "fast" codepath. I guess that just means I am not pushing it hard enough?
> > 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 :)
FWIW: This does work. I notice that this has to copy data into an allocated skb rather than building the skb around the data. I guess avoiding that copy is why the fast path existed in the first place?
Would you like me to post this patch (removing the fast path)?
Lincoln
next prev parent reply other threads:[~2020-11-19 5:20 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
2020-11-19 5:19 ` Ramsay, Lincoln [this message]
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CY4PR1001MB231125B16A35324A79270373E8E00@CY4PR1001MB2311.namprd10.prod.outlook.com \
--to=lincoln.ramsay@digi.com \
--cc=davem@davemloft.net \
--cc=dbogdanov@marvell.com \
--cc=irusskikh@marvell.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).