Re: [PATCH net-next v2 3/5] page_pool: Allow drivers to hint on SKB recycling

2021-04-09 Thread Jesper Dangaard Brouer
On Fri, 9 Apr 2021 22:01:51 +0300
Ilias Apalodimas  wrote:

> On Fri, Apr 09, 2021 at 11:56:48AM -0700, Jakub Kicinski wrote:
> > On Fri,  2 Apr 2021 20:17:31 +0200 Matteo Croce wrote:  
> > > Co-developed-by: Jesper Dangaard Brouer 
> > > Co-developed-by: Matteo Croce 
> > > Signed-off-by: Ilias Apalodimas   
> > 
> > Checkpatch says we need sign-offs from all authors.
> > Especially you since you're posting.  
> 
> Yes it does, we forgot that.  Let me take a chance on this one. 
> The patch is changing the default skb return path and while we've done enough
> testing, I would really prefer this going in on a future -rc1 (assuming we 
> even
> consider merging it), allowing enough time to have wider tests.

You can have my:

Signed-off-by: Jesper Dangaard Brouer 

But as Ilias suggested in IRC lets send a V3, and Cc the MM-people, as
this also dig into their area.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer



Re: [PATCH net-next v2 3/5] page_pool: Allow drivers to hint on SKB recycling

2021-04-09 Thread Ilias Apalodimas
On Fri, Apr 09, 2021 at 12:29:29PM -0700, Jakub Kicinski wrote:
> On Fri, 9 Apr 2021 22:01:51 +0300 Ilias Apalodimas wrote:
> > On Fri, Apr 09, 2021 at 11:56:48AM -0700, Jakub Kicinski wrote:
> > > On Fri,  2 Apr 2021 20:17:31 +0200 Matteo Croce wrote:  
> > > > Co-developed-by: Jesper Dangaard Brouer 
> > > > Co-developed-by: Matteo Croce 
> > > > Signed-off-by: Ilias Apalodimas   
> > > 
> > > Checkpatch says we need sign-offs from all authors.
> > > Especially you since you're posting.  
> > 
> > Yes it does, we forgot that.  Let me take a chance on this one. 
> > The patch is changing the default skb return path and while we've done 
> > enough
> > testing, I would really prefer this going in on a future -rc1 (assuming we 
> > even
> > consider merging it), allowing enough time to have wider tests.
> 
> Up to you guys. FWIW if you decide to try for 5.13 the missing signoffs
> can be posted in replies, no need to repost.
Thanks! but...
I think I prefer another repost, including mm-people on the list as well (and
fixing SoB's).
I just noticed noone is cc'ed and patch [2/5] adds a line in mm_types.h

/Ilias


Re: [PATCH net-next v2 3/5] page_pool: Allow drivers to hint on SKB recycling

2021-04-09 Thread Jakub Kicinski
On Fri, 9 Apr 2021 22:01:51 +0300 Ilias Apalodimas wrote:
> On Fri, Apr 09, 2021 at 11:56:48AM -0700, Jakub Kicinski wrote:
> > On Fri,  2 Apr 2021 20:17:31 +0200 Matteo Croce wrote:  
> > > Co-developed-by: Jesper Dangaard Brouer 
> > > Co-developed-by: Matteo Croce 
> > > Signed-off-by: Ilias Apalodimas   
> > 
> > Checkpatch says we need sign-offs from all authors.
> > Especially you since you're posting.  
> 
> Yes it does, we forgot that.  Let me take a chance on this one. 
> The patch is changing the default skb return path and while we've done enough
> testing, I would really prefer this going in on a future -rc1 (assuming we 
> even
> consider merging it), allowing enough time to have wider tests.

Up to you guys. FWIW if you decide to try for 5.13 the missing signoffs
can be posted in replies, no need to repost.


Re: [PATCH net-next v2 3/5] page_pool: Allow drivers to hint on SKB recycling

2021-04-09 Thread Ilias Apalodimas
On Fri, Apr 09, 2021 at 11:56:48AM -0700, Jakub Kicinski wrote:
> On Fri,  2 Apr 2021 20:17:31 +0200 Matteo Croce wrote:
> > Co-developed-by: Jesper Dangaard Brouer 
> > Co-developed-by: Matteo Croce 
> > Signed-off-by: Ilias Apalodimas 
> 
> Checkpatch says we need sign-offs from all authors.
> Especially you since you're posting.

Yes it does, we forgot that.  Let me take a chance on this one. 
The patch is changing the default skb return path and while we've done enough
testing, I would really prefer this going in on a future -rc1 (assuming we even
consider merging it), allowing enough time to have wider tests.

Regards
/Ilias


Re: [PATCH net-next v2 3/5] page_pool: Allow drivers to hint on SKB recycling

2021-04-09 Thread Jakub Kicinski
On Fri,  2 Apr 2021 20:17:31 +0200 Matteo Croce wrote:
> Co-developed-by: Jesper Dangaard Brouer 
> Co-developed-by: Matteo Croce 
> Signed-off-by: Ilias Apalodimas 

Checkpatch says we need sign-offs from all authors.
Especially you since you're posting.


[PATCH net-next v2 3/5] page_pool: Allow drivers to hint on SKB recycling

2021-04-02 Thread Matteo Croce
From: Ilias Apalodimas 

Up to now several high speed NICs have custom mechanisms of recycling
the allocated memory they use for their payloads.
Our page_pool API already has recycling capabilities that are always
used when we are running in 'XDP mode'.  So let's tweak the API and the
kernel network stack slightly and allow the recycling to happen even
during the standard operation.
The API doesn't take into account 'split page' policies used by those
drivers currently, but can be extended once we have users for that.

The idea is to be able to intercept the packet on skb_release_data().
If it's a buffer coming from our page_pool API recycle it back to the
pool for further usage or just release the packet entirely.

To achieve that we introduce a bit in struct sk_buff (pp_recycle:1) and
store the xdp_mem_info in page->private.  Storing the information in
page->private allows us to recycle both SKBs and their fragments.
The SKB bit is needed for a couple of reasons. First of all in an
effort to affect the free path as less as possible, reading a single bit,
is better that trying to derive identical information for the page stored
data. Moreover page->private is used by skb_copy_ubufs.  We do have a
special mark in the page, that won't allow this to happen, but again
deciding without having to read the entire page is preferable.

The driver has to take care of the sync operations on it's own
during the buffer recycling since the buffer is, after opting-in to the
recycling, never unmapped.

Since the gain on the drivers depends on the architecture, we are not
enabling recycling by default if the page_pool API is used on a driver.
In order to enable recycling the driver must call skb_mark_for_recycle()
to store the information we need for recycling in page->private and
enabling the recycling bit, or page_pool_store_mem_info() for a fragment

Since we added an extra argument on __skb_frag_unref() to handle
recycling, update the current users of the function with that.

Co-developed-by: Jesper Dangaard Brouer 
Co-developed-by: Matteo Croce 
Signed-off-by: Ilias Apalodimas 
---
 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c |  2 +-
 drivers/net/ethernet/marvell/sky2.c   |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c|  2 +-
 include/linux/skbuff.h| 35 ---
 include/net/page_pool.h   | 13 ++
 include/net/xdp.h |  1 +
 net/core/page_pool.c  | 43 +++
 net/core/skbuff.c | 20 -
 net/core/xdp.c|  6 +++
 net/tls/tls_device.c  |  2 +-
 10 files changed, 115 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c 
b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index 1115b8f9ea4e..8f815ebb59ae 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -2125,7 +2125,7 @@ static int chcr_ktls_xmit(struct sk_buff *skb, struct 
net_device *dev)
/* clear the frag ref count which increased locally before */
for (i = 0; i < record->num_frags; i++) {
/* clear the frag ref count */
-   __skb_frag_unref(&record->frags[i]);
+   __skb_frag_unref(&record->frags[i], false);
}
/* if any failure, come out from the loop. */
if (ret) {
diff --git a/drivers/net/ethernet/marvell/sky2.c 
b/drivers/net/ethernet/marvell/sky2.c
index 68c154d715d6..9dc25c4fb359 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2503,7 +2503,7 @@ static void skb_put_frags(struct sk_buff *skb, unsigned 
int hdr_space,
 
if (length == 0) {
/* don't need this page */
-   __skb_frag_unref(frag);
+   __skb_frag_unref(frag, false);
--skb_shinfo(skb)->nr_frags;
} else {
size = min(length, (unsigned) PAGE_SIZE);
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c 
b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index e35e4d7ef4d1..cea62b8f554c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -526,7 +526,7 @@ static int mlx4_en_complete_rx_desc(struct mlx4_en_priv 
*priv,
 fail:
while (nr > 0) {
nr--;
-   __skb_frag_unref(skb_shinfo(skb)->frags + nr);
+   __skb_frag_unref(skb_shinfo(skb)->frags + nr, false);
}
return 0;
 }
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c8def85fcc22..fffa6b35 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -40,6 +40,9 @@
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 #inclu