| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z6OMwvk9zANCD8jI@boxer>
Date: Wed, 5 Feb 2025 17:07:30 +0100
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: "Song, Yoong Siang" <yoong.siang.song@...el.com>
CC: "David S . Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
<pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Willem de Bruijn
<willemb@...gle.com>, "Bezdeka, Florian" <florian.bezdeka@...mens.com>,
Donald Hunter <donald.hunter@...il.com>, Jonathan Corbet <corbet@....net>,
Bjorn Topel <bjorn@...nel.org>, "Karlsson, Magnus"
<magnus.karlsson@...el.com>, Jonathan Lemon <jonathan.lemon@...il.com>,
Andrew Lunn <andrew+netdev@...n.ch>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer
<hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, "Damato, Joe"
<jdamato@...tly.com>, Stanislav Fomichev <sdf@...ichev.me>, Xuan Zhuo
<xuanzhuo@...ux.alibaba.com>, Mina Almasry <almasrymina@...gle.com>, "Daniel
Jurgens" <danielj@...dia.com>, Andrii Nakryiko <andrii@...nel.org>, "Eduard
Zingerman" <eddyz87@...il.com>, Mykola Lysenko <mykolal@...com>, "Martin
KaFai Lau" <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, Yonghong Song
<yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, Hao Luo
<haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, Shuah Khan
<shuah@...nel.org>, Alexandre Torgue <alexandre.torgue@...s.st.com>, "Jose
Abreu" <joabreu@...opsys.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, Faizal Rahim
<faizal.abdul.rahim@...ux.intel.com>, Choong Yong Liang
<yong.liang.choong@...ux.intel.com>, "Bouska, Zdenek"
<zdenek.bouska@...mens.com>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-doc@...r.kernel.org"
<linux-doc@...r.kernel.org>, "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "intel-wired-lan@...ts.osuosl.org"
<intel-wired-lan@...ts.osuosl.org>, "xdp-hints@...-project.net"
<xdp-hints@...-project.net>
Subject: Re: [PATCH bpf-next v8 4/5] igc: Refactor empty packet insertion
into a reusable function
On Wed, Feb 05, 2025 at 03:43:19PM +0100, Song, Yoong Siang wrote:
> On Wednesday, February 5, 2025 8:31 PM, Fijalkowski, Maciej <maciej.fijalkowski@...el.com> wrote:
> >On Wed, Feb 05, 2025 at 10:41:15AM +0800, Song Yoong Siang wrote:
> >> Refactor the code for inserting an empty packet into a new function
> >> igc_insert_empty_packet(). This change extracts the logic for inserting
> >> an empty packet from igc_xmit_frame_ring() into a separate function,
> >> allowing it to be reused in future implementations, such as the XDP
> >> zero copy transmit function.
> >>
> >> This patch introduces no functional changes.
> >>
> >> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> >
> >Your SoB should be last in the set of tags.
> >
>
> Noted. Thanks for the tips.
>
> >> Reviewed-by: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
> >> ---
> >> drivers/net/ethernet/intel/igc/igc_main.c | 42 ++++++++++++-----------
> >> 1 file changed, 22 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> >b/drivers/net/ethernet/intel/igc/igc_main.c
> >> index 21f318f12a8d..553d6d82af0d 100644
> >> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> >> @@ -1566,6 +1566,26 @@ static bool igc_request_tx_tstamp(struct igc_adapter
> >*adapter, struct sk_buff *s
> >> return false;
> >> }
> >>
> >> +static void igc_insert_empty_packet(struct igc_ring *tx_ring)
> >> +{
> >> + struct igc_tx_buffer *empty_info;
> >> + struct sk_buff *empty;
> >> + void *data;
> >> +
> >> + empty_info = &tx_ring->tx_buffer_info[tx_ring->next_to_use];
> >> + empty = alloc_skb(IGC_EMPTY_FRAME_SIZE, GFP_ATOMIC);
> >> + if (!empty)
> >> + return;
> >> +
> >> + data = skb_put(empty, IGC_EMPTY_FRAME_SIZE);
> >> + memset(data, 0, IGC_EMPTY_FRAME_SIZE);
> >> +
> >> + igc_tx_ctxtdesc(tx_ring, 0, false, 0, 0, 0);
> >> +
> >> + if (igc_init_tx_empty_descriptor(tx_ring, empty, empty_info) < 0)
> >> + dev_kfree_skb_any(empty);
> >
> >I still don't like the fact igc_insert_empty_packet() doesn't communicate
> >to caller whether it successfully produced descriptors or not.
> >
> >Look at this from igc_xmit_frame_ring() POV:
> >- at the beginning you peek at Tx ring whether there is required amount of
> > descriptors free to be used
> >- but then here's your additional routine which might consume two more
> > descs and you are not aware of the status
> >- then you continue to further produce descriptors assuming there is
> > enough space in Tx ring
> >
> >Right now igc_init_tx_empty_descriptor() returns -EBUSY when ring is full.
> >How can that happen in the first place + what if it would *really* happen
> >though? You just continue with your Tx flow.
> >
> >What I'm trying to say here is, at least from correctness POV, you should
> >take into the account two potential descriptors for launchtime feature
> >when calling igc_maybe_stop_tx(). And igc_init_tx_empty_descriptor()
> >should not really care about space in ring, it should be a caller's job to
> >call it only when it will be sure it's safe to do so.
> >
>
> Agree with you.
>
> In db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit"),
> the 2 descriptors needed by empty packet is already taken into
> consideration by changing igc_maybe_stop_tx(tx_ring, count + 3) to
> igc_maybe_stop_tx(tx_ring, count + 5), so not enough ring space issue will not
> happened. However, the comment session is not updated, maybe i can update
> it in next version of this patch as below:
>
> @@ -1586,6 +1608,7 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
> * + 1 desc for skb_headlen/IGC_MAX_DATA_PER_TXD,
> * + 2 desc gap to keep tail from touching head,
> * + 1 desc for context descriptor,
> + * + 2 desc for inserting an empty packet for launch time,
> * otherwise try next time
> */
> for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
Ahh good then, I didn't pay enough attention to comment. So it meant that
first two entries from comment were covered by @count and 2 desc gap + 1
ctxt desc were behind '3', previously.
>
> Since number of descriptor needed is guaranteed. I will take out the
> igc_desc_unused() checking in igc_init_tx_empty_descriptor().
Yes
> However, empty packet insertion might fail due to skb allocation failure
> and DMA mapping error. How about i make sure skb allocation and DMA
> mapping working, before proceed to fill in igc_tx_buffer, context desc,
> and data desc?
> IMHO, because these two errors are unlikely to happen, print a kernel
> warning msg should be enough.
>
> @@ -1108,20 +1108,12 @@ static int igc_init_empty_frame(struct igc_ring *ring,
> return 0;
> }
>
> -static int igc_init_tx_empty_descriptor(struct igc_ring *ring,
> +static void igc_init_tx_empty_descriptor(struct igc_ring *ring,
> struct sk_buff *skb,
> struct igc_tx_buffer *first)
> {
> union igc_adv_tx_desc *desc;
> u32 cmd_type, olinfo_status;
> - int err;
> -
> - if (!igc_desc_unused(ring))
> - return -EBUSY;
> -
> - err = igc_init_empty_frame(ring, first, skb);
> - if (err)
> - return err;
>
> cmd_type = IGC_ADVTXD_DTYP_DATA | IGC_ADVTXD_DCMD_DEXT |
> IGC_ADVTXD_DCMD_IFCS | IGC_TXD_DCMD |
> @@ -1140,8 +1132,6 @@ static int igc_init_tx_empty_descriptor(struct igc_ring *ring,
> ring->next_to_use++;
> if (ring->next_to_use == ring->count)
> ring->next_to_use = 0;
> -
> - return 0;
> }
>
> #define IGC_EMPTY_FRAME_SIZE 60
> @@ -1567,6 +1557,38 @@ static bool igc_request_tx_tstamp(struct igc_adapter *adapter, struct sk_buff *s
> return false;
> }
>
> +static void igc_insert_empty_packet(struct igc_ring *tx_ring)
> +{
> + struct igc_tx_buffer *empty_info;
> + struct sk_buff *empty;
> + void *data;
> +
> + empty_info = &tx_ring->tx_buffer_info[tx_ring->next_to_use];
> + empty = alloc_skb(IGC_EMPTY_FRAME_SIZE, GFP_ATOMIC);
> + if (unlikely(!empty)) {
> + netdev_warn(tx_ring->netdev,
> + "Fail to alloc skb for empty packet\n");
That should be at least ratelimited I think, but what really is the
problem to make these routines return status and check them on caller
side?
> + return;
> + }
> +
> + data = skb_put(empty, IGC_EMPTY_FRAME_SIZE);
> + memset(data, 0, IGC_EMPTY_FRAME_SIZE);
> +
> + /* Prepare DMA mapping and Tx buffer information */
> + if (unlikely(igc_init_empty_frame(tx_ring, empty_info, empty))) {
> + dev_kfree_skb_any(empty);
> + netdev_warn(tx_ring->netdev,
> + "Fail to map DMA for empty packet\n");
> + return;
> + }
> +
> + /* Prepare context descriptor for empty packet */
> + igc_tx_ctxtdesc(tx_ring, 0, false, 0, 0, 0);
> +
> + /* Prepare data descriptor for empty packet */
> + igc_init_tx_empty_descriptor(tx_ring, empty, empty_info);
> +}
> +
> static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
> struct igc_ring *tx_ring)
> {
>
> Are above codes resolve your concern? If yes, i can test and add
> them into this patch in next version submission.
>
> Thanks & Regards
> Siang
Powered by blists - more mailing lists