RE: [PATCH bpf-next v7 4/5] igc: Refactor empty packet insertion into a reusable function

[Song, Yoong Siang]

On Tuesday, February 4, 2025 5:50 PM, Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx> wrote:
>On Tue, Feb 04, 2025 at 08:49:06AM +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@xxxxxxxxx>
>> ---
>>  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 56a35d58e7a6..c3edd8bcf633 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;
>
>shouldn't this be 'goto drop' from day 1? pretty weird to silently ignore
>allocation error.
>

Hi Fijalkowski Maciej,

Thanks for your comments.

"insert an empty packet" is a launch time trick to send a packet in
next Qbv cycle. The design is, the driver will still sending the
packet, even the empty packet insertion trick is fail (unable to
allocate). The intention of this patch set is to enable launch time
on XDP zero-copy data path, so I try not to change the original
behavior of launch time.

btw, do you think driver should drop the packet if something went
wrong with the launch time, like launch time offload not enabled,
launch time over horizon, empty packet insertion fail, etc?
If yes, then maybe i can submit another patch to change the behavior
of launch time and we can continue to discuss there.

>> -
>> -		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);
>
>ditto
>

ditto

>> -	}
>> +	if (insert_empty)
>> +		igc_insert_empty_packet(tx_ring);
>>
>>  done:
>>  	/* record the location of the first descriptor for this packet */
>> --
>> 2.34.1
>>

Thanks & Regards
Siang