[bpf-next,v8,4/5] igc: Refactor empty packet insertion into a reusable function

Commit Message

Song, Yoong Siang Feb. 5, 2025, 2:41 a.m. UTC

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 <[e-mail]>
Reviewed-by: Faizal Rahim <[e-mail]>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 42 ++++++++++++-----------
 1 file changed, 22 insertions(+), 20 deletions(-)

Comments

Maciej Fijalkowski Feb. 5, 2025, 12:31 p.m. UTC | #1

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 <[e-mail]>

Your SoB should be last in the set of tags.

> Reviewed-by: Faizal Rahim <[e-mail]>
> ---
>  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.

> +}
> +
>  static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  				       struct igc_ring *tx_ring)
>  {
> @@ -1603,26 +1623,8 @@ static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
>  	skb->tstamp = ktime_set(0, 0);
>  	launch_time = igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
>  
> -	if (insert_empty) {
> -		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)
> -			goto done;
> -
> -		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);
> -	}
> +	if (insert_empty)
> +		igc_insert_empty_packet(tx_ring);
>  
>  done:
>  	/* record the location of the first descriptor for this packet */
> -- 
> 2.34.1
>

Song, Yoong Siang Feb. 5, 2025, 2:43 p.m. UTC | #2

On Wednesday, February 5, 2025 8:31 PM, Fijalkowski, Maciej <[e-mail]> 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 <[e-mail]>
>
>Your SoB should be last in the set of tags.
>

Noted. Thanks for the tips.

>> Reviewed-by: Faizal Rahim <[e-mail]>
>> ---
>>  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

Maciej Fijalkowski Feb. 5, 2025, 4:07 p.m. UTC | #3

On Wed, Feb 05, 2025 at 03:43:19PM +0100, Song, Yoong Siang wrote:
> On Wednesday, February 5, 2025 8:31 PM, Fijalkowski, Maciej <[e-mail]> 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 <[e-mail]>
> >
> >Your SoB should be last in the set of tags.
> >
> 
> Noted. Thanks for the tips.
> 
> >> Reviewed-by: Faizal Rahim <[e-mail]>
> >> ---
> >>  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

Patch

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);
+}
+
 static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 				       struct igc_ring *tx_ring)
 {
@@ -1603,26 +1623,8 @@  static netdev_tx_t igc_xmit_frame_ring(struct sk_buff *skb,
 	skb->tstamp = ktime_set(0, 0);
 	launch_time = igc_tx_launchtime(tx_ring, txtime, &first_flag, &insert_empty);
 
-	if (insert_empty) {
-		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)
-			goto done;
-
-		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);
-	}
+	if (insert_empty)
+		igc_insert_empty_packet(tx_ring);
 
 done:
 	/* record the location of the first descriptor for this packet */