net: veth: Page pool creation error handling for existing pools only

drivers/net/veth.c +6 -0

+		} else {
+			veth_xdp_get(xdp);
+			consume_skb(skb);
+			xdp->rxq->mem = rq->xdp_mem;
+		}
+

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 870

Quoted 10 lines · Liang Chen

drivers/net/veth.c +3 -3

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
      case XDP_PASS:
              break;
      case XDP_TX:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;
+                     kfree_skb_partial(skb, true);

For this case, I suppose that we can safely call kfree_skb_partial() as we allocate the skb in the veth_convert_skb_to_xdp_buff(), but I am not sure about the !skb->pp_recycle case.

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 873

Quoted 3 lines

drivers/net/veth.c +3 -0

+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);

For consume_skb(), there is skb_unref() checking and other checking/operation. Can we really assume that we can call kfree_skb_partial() with head_stolen being true? Is it possible that skb->users is bigger than 1? If it is possible, don't we free the 'skb' back to skbuff_cache when other may still be using it?

Quoted 6 lines

drivers/net/veth.c +6 -0

+             } else {
+                     veth_xdp_get(xdp);
+                     consume_skb(skb);
+                     xdp->rxq->mem = rq->xdp_mem;
+             }
+

Quoted 22 lines · Yunsheng Lin

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 870

Quoted 10 lines · Liang Chen

drivers/net/veth.c +3 -3

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
      case XDP_PASS:
              break;
      case XDP_TX:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;
+                     kfree_skb_partial(skb, true);

For this case, I suppose that we can safely call kfree_skb_partial() as we allocate the skb in the veth_convert_skb_to_xdp_buff(), but I am not sure about the !skb->pp_recycle case.

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 873

Quoted 3 lines

drivers/net/veth.c +3 -0

+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);

Quoted 6 lines

For consume_skb(), there is skb_unref() checking and other checking/operation. Can we really assume that we can call kfree_skb_partial() with head_stolen being true? Is it possible that skb->users is bigger than 1? If it is possible, don't we free the 'skb' back to skbuff_cache when other may still be using it?

Thanks for raising the concern. If there are multiple references to the skb (skb->users is greater than 1), the skb will be reallocated in veth_convert_skb_to_xdp_buff(). So it should enter the skb != skb_orig case.

In fact, entering the !skb->pp_recycle case implies that the skb meets the following conditions:
1. It is neither shared nor cloned.
2. It is not allocated using kmalloc.
3. It does not have fragment data.
4. The headroom of the skb is greater than XDP_PACKET_HEADROOM.

Quoted 35 lines · Liang Chen

Quoted 22 lines · Yunsheng Lin

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 870

Quoted 10 lines · Liang Chen

drivers/net/veth.c +3 -3

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
      case XDP_PASS:
              break;
      case XDP_TX:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;
+                     kfree_skb_partial(skb, true);

For this case, I suppose that we can safely call kfree_skb_partial() as we allocate the skb in the veth_convert_skb_to_xdp_buff(), but I am not sure about the !skb->pp_recycle case.

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 873

Quoted 3 lines

drivers/net/veth.c +3 -0

+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);

Quoted 6 lines

For consume_skb(), there is skb_unref() checking and other checking/operation. Can we really assume that we can call kfree_skb_partial() with head_stolen being true? Is it possible that skb->users is bigger than 1? If it is possible, don't we free the 'skb' back to skbuff_cache when other may still be using it?

Thanks for raising the concern. If there are multiple references to the skb (skb->users is greater than 1), the skb will be reallocated in veth_convert_skb_to_xdp_buff(). So it should enter the skb != skb_orig case.

Quoted 7 lines

In fact, entering the !skb->pp_recycle case implies that the skb meets the following conditions:
1. It is neither shared nor cloned.
2. It is not allocated using kmalloc.
3. It does not have fragment data.
4. The headroom of the skb is greater than XDP_PACKET_HEADROOM.

You are right, I missed the checking in veth_convert_skb_to_xdp_buff(), it seems the xdp is pretty strict about the buffer owner, it need to have exclusive access to all the buffer.

And it seems there is only one difference left then, with kfree_skb_partial() calling 'kmem_cache_free(skbuff_cache, skb)' and consume_skb() calling 'kfree_skbmem(skb)'. If we are true about 'skb' only allocated from 'skbuff_cache', this patch looks good to me then.

Quoted 1 line

Quoted 49 lines · Yunsheng Lin

Quoted 35 lines · Liang Chen

Quoted 22 lines · Yunsheng Lin

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 870

Quoted 10 lines · Liang Chen

drivers/net/veth.c +3 -3

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
      case XDP_PASS:
              break;
      case XDP_TX:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;
+                     kfree_skb_partial(skb, true);

For this case, I suppose that we can safely call kfree_skb_partial() as we allocate the skb in the veth_convert_skb_to_xdp_buff(), but I am not sure about the !skb->pp_recycle case.

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 873

Quoted 3 lines

drivers/net/veth.c +3 -0

+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);

Quoted 6 lines

For consume_skb(), there is skb_unref() checking and other checking/operation. Can we really assume that we can call kfree_skb_partial() with head_stolen being true? Is it possible that skb->users is bigger than 1? If it is possible, don't we free the 'skb' back to skbuff_cache when other may still be using it?

Thanks for raising the concern. If there are multiple references to the skb (skb->users is greater than 1), the skb will be reallocated in veth_convert_skb_to_xdp_buff(). So it should enter the skb != skb_orig case.

Quoted 7 lines

In fact, entering the !skb->pp_recycle case implies that the skb meets the following conditions:
1. It is neither shared nor cloned.
2. It is not allocated using kmalloc.
3. It does not have fragment data.
4. The headroom of the skb is greater than XDP_PACKET_HEADROOM.

You are right, I missed the checking in veth_convert_skb_to_xdp_buff(), it seems the xdp is pretty strict about the buffer owner, it need to have exclusive access to all the buffer.

Quoted 6 lines

And it seems there is only one difference left then, with kfree_skb_partial() calling 'kmem_cache_free(skbuff_cache, skb)' and consume_skb() calling 'kfree_skbmem(skb)'. If we are true about 'skb' only allocated from 'skbuff_cache', this patch looks good to me then.

The difference between kmem_cache_free and kfree_skbmem lies in the fact that kfree_skbmem checks whether the skb is an fclone (fast clone) skb. If it is, it should be returned to the skbuff_fclone_cache. Currently, fclone skbs can only be allocated through __alloc_skb, and their head buffer is allocated by kmalloc_reserve, which does not meet the condition mentioned above - "2. It is not allocated using kmalloc.". Therefore, the fclone skb will still be reallocated by veth_convert_skb_to_xdp_buff, leading to the skb != skb_orig case. In other words, entering the !skb->pp_recycle case indicates that the skb was allocated from skbuff_cache.

Quoted 57 lines · Liang Chen

Quoted 49 lines · Yunsheng Lin

Quoted 35 lines · Liang Chen

Quoted 22 lines · Yunsheng Lin

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 870

Quoted 10 lines · Liang Chen

drivers/net/veth.c +3 -3

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
      case XDP_PASS:
              break;
      case XDP_TX:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;
+                     kfree_skb_partial(skb, true);

For this case, I suppose that we can safely call kfree_skb_partial() as we allocate the skb in the veth_convert_skb_to_xdp_buff(), but I am not sure about the !skb->pp_recycle case.

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 873

Quoted 3 lines

drivers/net/veth.c +3 -0

+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);

Quoted 6 lines

For consume_skb(), there is skb_unref() checking and other checking/operation. Can we really assume that we can call kfree_skb_partial() with head_stolen being true? Is it possible that skb->users is bigger than 1? If it is possible, don't we free the 'skb' back to skbuff_cache when other may still be using it?

Thanks for raising the concern. If there are multiple references to the skb (skb->users is greater than 1), the skb will be reallocated in veth_convert_skb_to_xdp_buff(). So it should enter the skb != skb_orig case.

Quoted 7 lines

In fact, entering the !skb->pp_recycle case implies that the skb meets the following conditions:
1. It is neither shared nor cloned.
2. It is not allocated using kmalloc.
3. It does not have fragment data.
4. The headroom of the skb is greater than XDP_PACKET_HEADROOM.

You are right, I missed the checking in veth_convert_skb_to_xdp_buff(), it seems the xdp is pretty strict about the buffer owner, it need to have exclusive access to all the buffer.

Quoted 6 lines

And it seems there is only one difference left then, with kfree_skb_partial() calling 'kmem_cache_free(skbuff_cache, skb)' and consume_skb() calling 'kfree_skbmem(skb)'. If we are true about 'skb' only allocated from 'skbuff_cache', this patch looks good to me then.

Quoted 11 lines

The difference between kmem_cache_free and kfree_skbmem lies in the fact that kfree_skbmem checks whether the skb is an fclone (fast clone) skb. If it is, it should be returned to the skbuff_fclone_cache. Currently, fclone skbs can only be allocated through __alloc_skb, and their head buffer is allocated by kmalloc_reserve, which does not meet the condition mentioned above - "2. It is not allocated using kmalloc.". Therefore, the fclone skb will still be reallocated by veth_convert_skb_to_xdp_buff, leading to the skb != skb_orig case. In other words, entering the !skb->pp_recycle case indicates that the skb was allocated from skbuff_cache.

It might need some comment to make it clear or add some compile testing such as BUILD_BUG_ON() to ensure that, as it is not so obvious if someone change it to allocate a fclone skb with a frag head data in the future.

Also I suppose the veth_xdp_rcv_skb() is called in NAPI context, and we might be able to reuse the 'skb' if we can use something like napi_skb_free_stolen_head().

drivers/net/veth.c +0 -0

index 509e901da41d..ea1b344e5db4 100644

drivers/net/veth.c +3 -3

@@ -848,6 +850,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
  		goto out;
  	}
 
+	skb_orig = skb;
  	__skb_push(skb, skb->data - skb_mac_header(skb));
  	if (veth_convert_skb_to_xdp_buff(rq, xdp, &skb))
  		goto drop;

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
  	case XDP_PASS:
  		break;
  	case XDP_TX:
-		veth_xdp_get(xdp);
-		consume_skb(skb);
-		xdp->rxq->mem = rq->xdp_mem;
+		if (skb != skb_orig) {
+			xdp->rxq->mem = rq->xdp_mem_pp;

drivers/net/veth.c +2 -3

  		if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
  			trace_xdp_exception(rq->dev, xdp_prog, act);
  			stats->rx_drops++;

@@ -874,9 +886,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
  		rcu_read_unlock();
  		goto xdp_xmit;
  	case XDP_REDIRECT:
-		veth_xdp_get(xdp);
-		consume_skb(skb);
-		xdp->rxq->mem = rq->xdp_mem;
+		if (skb != skb_orig) {
+			xdp->rxq->mem = rq->xdp_mem_pp;

On 01/08/2023 08.19, Liang Chen wrote:
[...]

Quoted 4 lines

drivers/net/veth.c +0 -0

index 509e901da41d..ea1b344e5db4 100644

[...]

Quoted 17 lines

drivers/net/veth.c +3 -3

@@ -848,6 +850,7 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
              goto out;
      }
 
+     skb_orig = skb;
      __skb_push(skb, skb->data - skb_mac_header(skb));
      if (veth_convert_skb_to_xdp_buff(rq, xdp, &skb))
              goto drop;

@@ -862,9 +865,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
      case XDP_PASS:
              break;
      case XDP_TX:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 879

Quoted 10 lines

drivers/net/veth.c +10 -0

+                     kfree_skb_partial(skb, true);
+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);
+             } else {
+                     veth_xdp_get(xdp);
+                     consume_skb(skb);
+                     xdp->rxq->mem = rq->xdp_mem;
+             }
+

Above code section, and below section looks the same. It begs for a common function.

Quoted 12 lines

drivers/net/veth.c +2 -3

              if (unlikely(veth_xdp_tx(rq, xdp, bq) < 0)) {
                      trace_xdp_exception(rq->dev, xdp_prog, act);
                      stats->rx_drops++;

@@ -874,9 +886,18 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
              rcu_read_unlock();
              goto xdp_xmit;
      case XDP_REDIRECT:
-             veth_xdp_get(xdp);
-             consume_skb(skb);
-             xdp->rxq->mem = rq->xdp_mem;
+             if (skb != skb_orig) {
+                     xdp->rxq->mem = rq->xdp_mem_pp;

drivers/net/veth.c static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq, · line 900

Quoted 10 lines

drivers/net/veth.c +10 -0

+                     kfree_skb_partial(skb, true);
+             } else if (!skb->pp_recycle) {
+                     xdp->rxq->mem = rq->xdp_mem;
+                     kfree_skb_partial(skb, true);
+             } else {
+                     veth_xdp_get(xdp);
+                     consume_skb(skb);
+                     xdp->rxq->mem = rq->xdp_mem;
+             }
+

The common function can be named to reflect what the purpose of this code section is. According to my understanding, the code steals the (packet) data section from the SKB and free the SKB. And prepare/associate the correct memory type in xdp_buff->rxq.

Function name proposals:
__skb_steal_data
__free_skb_and_steal_data
__free_skb_and_steal_data_for_xdp
__free_skb_and_xdp_steal_data
__skb2xdp_steal_data

When doing this in a function, it will also allow us to add some comments explaining the different cases and assumptions. These assumptions can get broken as a result of (future) changes in surrounding the code, thus we need to explain our assumptions/intent (to help our future selves).

For code readability, I think we should convert (skb != skb_orig) into a boolean that says what this case captures, e.g. local_pp_alloc.

Func prototype:
__skb2xdp_steal_data(skb, xdp, rq, bool local_pp_alloc)

Always feel free to challenge my view,
--Jesper

The failure handling procedure destroys page pools for all queues, including those that haven't had their page pool created yet. this patch introduces necessary adjustments to prevent potential risks and inconsistency with the error handling behavior.

Signed-off-by Liang Chen <[email protected]>

Quoted 6 lines · Liang Chen

The failure handling procedure destroys page pools for all queues, including those that haven't had their page pool created yet. this patch introduces necessary adjustments to prevent potential risks and inconsistency with the error handling behavior.

Signed-off-by Liang Chen <[email protected]>

I wonder if this should this have a fixes tag and be targeted at 'net' as a bugfix?

The failure handling procedure destroys page pools for all queues, including those that haven't had their page pool created yet. this patch introduces necessary adjustments to prevent potential risks and inconsistency with the error handling behavior.

Signed-off-by Liang Chen <[email protected]>

---
  drivers/net/veth.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/veth.c +0 -0

index 614f3e3efab0..509e901da41d 100644

Quoted 15 lines · Liang Chen

The failure handling procedure destroys page pools for all queues, including those that haven't had their page pool created yet. this patch introduces necessary adjustments to prevent potential risks and inconsistency with the error handling behavior.

Signed-off-by Liang Chen <[email protected]>

---
  drivers/net/veth.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/net/veth.c +0 -0

index 614f3e3efab0..509e901da41d 100644

drivers/net/veth.c static int __veth_napi_enable_range(struct net_device *dev, int start, int end) · line 1086

Quoted 7 lines

drivers/net/veth.c +2 -1

@@ -1081,8 +1081,9 @@ static int __veth_napi_enable_range(struct net_device *dev, int start, int end)
  err_xdp_ring:
      for (i--; i >= start; i--)
              ptr_ring_cleanup(&priv->rq[i].xdp_ring, veth_ptr_free);
+     i = end;
  err_page_pool:
-     for (i = start; i < end; i++) {
+     for (i--; i >= start; i--) {

I'm not a fan of this coding style, that iterates backwards, but I can see you just inherited the existing style in this function.

drivers/net/veth.c static int __veth_napi_enable_range(struct net_device *dev, int start, int end) · line 1089

Quoted 3 lines

drivers/net/veth.c +0 -0

              page_pool_destroy(priv->rq[i].page_pool);
              priv->rq[i].page_pool = NULL;
      }

The page_pool_destroy() call handles(exits) if called with NULL. So, I don't think this incorrect walking all (start to end) can trigger an actual bug.

Anyhow, I do think this is more correct, so you can append my ACK for the real submission.

Acked-by Jesper Dangaard Brouer <[email protected]>

Quoted 1 line · Jesper Dangaard Brouer

Quoted 11 lines

Quoted 3 lines

               page_pool_destroy(priv->rq[i].page_pool);
               priv->rq[i].page_pool = NULL;
       }

The page_pool_destroy() call handles(exits) if called with NULL. So, I don't think this incorrect walking all (start to end) can trigger an actual bug.

Anyhow, I do think this is more correct, so you can append my ACK for the real submission.

Quoted 2 lines

Acked-by Jesper Dangaard Brouer <[email protected]>

Thanks! I will separate this patch out and make a real submission, since it's a small fix and not really coupled with the optimization patch which still needs some further work after receiving feedback from Yunsheng.

Quoted 2 lines · Liang Chen

Quoted 1 line · Jesper Dangaard Brouer

[...]

Quoted 11 lines

Quoted 11 lines

Quoted 3 lines

               page_pool_destroy(priv->rq[i].page_pool);
               priv->rq[i].page_pool = NULL;
       }

The page_pool_destroy() call handles(exits) if called with NULL. So, I don't think this incorrect walking all (start to end) can trigger an actual bug.

Anyhow, I do think this is more correct, so you can append my ACK for the real submission.

Quoted 7 lines

Quoted 2 lines

Acked-by Jesper Dangaard Brouer <[email protected]>

Thanks! I will separate this patch out and make a real submission, since it's a small fix and not really coupled with the optimization patch which still needs some further work after receiving feedback from Yunsheng.

Sure, send it as a fix commit. Given it is not super critical i think it is okay to send for net-next, to avoid merge issues/conflicts with your 2/2 optimization patch. And for good order we should add a Fixes tag, but IMHO net-next is still okay, given I don't think this can trigger a bug.

That said, I do want to encourage you to work on 2/2 optimization patch. I think this is a very important optimization and actually a fix for then we introduced page_pool to veth. Well spotted! :-)

In 2/2 I keep getting confused by use of kfree_skb_partial() and if the right conditions are meet.

--Jesper