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, Maciejmaciej.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