[xdp-hints] Re: [PATCH bpf-next v8 4/5] igc: Refactor empty packet insertion into a reusable function

["Song, Yoong Siang" <yoong.siang.song@intel.com>]

On Thursday, February 6, 2025 12:08 AM, Fijalkowski, Maciej <maciej.fijalkowski@intel.com> wrote:
>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@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++)
>
>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.
>

Yes, you are right.

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

No problem, I will add net_ratelimit() checking.

>problem to make these routines return status and check them on caller
>side?
>

Ok, I will add return value and handle the case where the insertion of
an empty frame fails and explain the reason behind.

Thanks & Regards
Siang