On Wednesday, February 5, 2025 8:31 PM, Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx> 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@xxxxxxxxx>
>
>Your SoB should be last in the set of tags.
>
Noted. Thanks for the tips.
>> Reviewed-by: Faizal Rahim <faizal.abdul.rahim@xxxxxxxxxxxxxxx>
>> ---
>> 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
