XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: "Song, Yoong Siang" <yoong.siang.song@intel.com>
To: "Fijalkowski, Maciej" <maciej.fijalkowski@intel.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Simon Horman <horms@kernel.org>,
	Willem de Bruijn <willemb@google.com>,
	"Bezdeka, Florian" <florian.bezdeka@siemens.com>,
	Donald Hunter <donald.hunter@gmail.com>,
	Jonathan Corbet <corbet@lwn.net>, Bjorn Topel <bjorn@kernel.org>,
	"Karlsson, Magnus" <magnus.karlsson@intel.com>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	Andrew Lunn <andrew+netdev@lunn.ch>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	"Damato, Joe" <jdamato@fastly.com>,
	Stanislav Fomichev <sdf@fomichev.me>,
	Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
	Mina Almasry <almasrymina@google.com>,
	Daniel Jurgens <danielj@nvidia.com>,
	Andrii Nakryiko <andrii@kernel.org>,
	Eduard Zingerman <eddyz87@gmail.com>,
	Mykola Lysenko <mykolal@fb.com>,
	Martin KaFai Lau <martin.lau@linux.dev>,
	Song Liu <song@kernel.org>,
	Yonghong Song <yonghong.song@linux.dev>,
	KP Singh <kpsingh@kernel.org>, Hao Luo <haoluo@google.com>,
	Jiri Olsa <jolsa@kernel.org>, Shuah Khan <shuah@kernel.org>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@intel.com>,
	Faizal Rahim <faizal.abdul.rahim@linux.intel.com>,
	Choong Yong Liang <yong.liang.choong@linux.intel.com>,
	"Bouska, Zdenek" <zdenek.bouska@siemens.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com"
	<linux-stm32@st-md-mailman.stormreply.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>,
	"xdp-hints@xdp-project.net" <xdp-hints@xdp-project.net>
Subject: [xdp-hints] Re: [PATCH bpf-next v8 4/5] igc: Refactor empty packet insertion into a reusable function
Date: Wed, 5 Feb 2025 14:43:19 +0000	[thread overview]
Message-ID: <PH0PR11MB58305176708A48DD8EA60B96D8F72@PH0PR11MB5830.namprd11.prod.outlook.com> (raw)
In-Reply-To: <Z6NaFkPBZA18oILE@boxer>

On Wednesday, February 5, 2025 8:31 PM, Fijalkowski, Maciej <maciej.fijalkowski@intel.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@intel.com>
>
>Your SoB should be last in the set of tags.
>

Noted. Thanks for the tips.

>> Reviewed-by: Faizal Rahim <faizal.abdul.rahim@linux.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++)

Since number of descriptor needed is guaranteed. I will take out the
igc_desc_unused() checking in igc_init_tx_empty_descriptor().
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");
+               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

  reply	other threads:[~2025-02-05 14:43 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-05  2:41 [xdp-hints] [PATCH bpf-next v8 0/5] xsk: TX metadata Launch Time support Song Yoong Siang
2025-02-05  2:41 ` [xdp-hints] [PATCH bpf-next v8 1/5] xsk: Add launch time hardware offload support to XDP Tx metadata Song Yoong Siang
2025-02-05  2:41 ` [xdp-hints] [PATCH bpf-next v8 2/5] selftests/bpf: Add launch time request to xdp_hw_metadata Song Yoong Siang
2025-02-05  2:41 ` [xdp-hints] [PATCH bpf-next v8 3/5] net: stmmac: Add launch time support to XDP ZC Song Yoong Siang
2025-02-05  2:41 ` [xdp-hints] [PATCH bpf-next v8 4/5] igc: Refactor empty packet insertion into a reusable function Song Yoong Siang
2025-02-05 12:31   ` [xdp-hints] " Maciej Fijalkowski
2025-02-05 14:43     ` Song, Yoong Siang [this message]
2025-02-05 16:07       ` Maciej Fijalkowski
2025-02-06  4:32         ` Song, Yoong Siang
2025-02-05  2:41 ` [xdp-hints] [PATCH bpf-next v8 5/5] igc: Add launch time support to XDP ZC Song Yoong Siang

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

  List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/

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

  git send-email \
    --in-reply-to=PH0PR11MB58305176708A48DD8EA60B96D8F72@PH0PR11MB5830.namprd11.prod.outlook.com \
    --to=yoong.siang.song@intel.com \
    --cc=alexandre.torgue@foss.st.com \
    --cc=almasrymina@google.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=andrii@kernel.org \
    --cc=anthony.l.nguyen@intel.com \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=danielj@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=donald.hunter@gmail.com \
    --cc=eddyz87@gmail.com \
    --cc=edumazet@google.com \
    --cc=faizal.abdul.rahim@linux.intel.com \
    --cc=florian.bezdeka@siemens.com \
    --cc=haoluo@google.com \
    --cc=hawk@kernel.org \
    --cc=horms@kernel.org \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=jdamato@fastly.com \
    --cc=joabreu@synopsys.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=jonathan.lemon@gmail.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@intel.com \
    --cc=martin.lau@linux.dev \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=mykolal@fb.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=przemyslaw.kitszel@intel.com \
    --cc=sdf@fomichev.me \
    --cc=shuah@kernel.org \
    --cc=song@kernel.org \
    --cc=willemb@google.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=xuanzhuo@linux.alibaba.com \
    --cc=yong.liang.choong@linux.intel.com \
    --cc=yonghong.song@linux.dev \
    --cc=zdenek.bouska@siemens.com \
    /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