add multi-buff support for xdp running in generic mode

net/core/dev.c +11 -0

+		return -ENOMEM;
+	}
+
+	per_cpu(page_pool, cpuid) = pp_ptr;
+#endif
+	return 0;
+}
+
 /*
  *       This is called single threaded during boot, so no need
  *       to take the rtnl semaphore.

@@ -11738,6 +11763,9 @@ static int __init net_dev_init(void)
 		init_gro_hash(&sd->backlog);
 		sd->backlog.poll = process_backlog;
 		sd->backlog.weight = weight_p;
+
+		if (net_page_pool_alloc(i))
+			goto out;
 	}
 
 	dev_boot_phase = 0;

net/core/dev.c +9 -0

+
+			if (!pp_ptr)
+				continue;
+
+			page_pool_destroy(pp_ptr);
+			per_cpu(page_pool, i) = NULL;
+		}
+	}
+
 	return rc;
 }

Quoted 8 lines

#ifdef CONFIG_LOCKDEP
/*
* register_netdevice() inits txq->_xmit_lock and sets lockdep class

(quoted diff) +0 -0

@@ -11686,6 +11690,27 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Review static void __init net_dev_struct_check(void) · line 11693

Quoted 1 line

(quoted diff) +1 -0

+#define SD_PAGE_POOL_RING_SIZE	256

I might missed that if there is a reason we choose 256 here, do we need to use different value for differe page size, for 64K page size, it means we might need to reserve 16MB memory for each CPU.

Review static void __init net_dev_struct_check(void) · line 11705

Quoted 13 lines

(quoted diff) +12 -0

+static int net_page_pool_alloc(int cpuid)
+{
+#if IS_ENABLED(CONFIG_PAGE_POOL)
+	struct page_pool_params page_pool_params = {
+		.pool_size = SD_PAGE_POOL_RING_SIZE,
+		.nid = NUMA_NO_NODE,
+	};
+	struct page_pool *pp_ptr;
+
+	pp_ptr = page_pool_create_percpu(&page_pool_params, cpuid);
+	if (IS_ERR(pp_ptr)) {
+		pp_ptr = NULL;

unnecessary NULL setting?

Quoted 21 lines

(quoted diff) +11 -0

+		return -ENOMEM;
+	}
+
+	per_cpu(page_pool, cpuid) = pp_ptr;
+#endif
+	return 0;
+}
+
 /*
  *       This is called single threaded during boot, so no need
  *       to take the rtnl semaphore.

@@ -11738,6 +11763,9 @@ static int __init net_dev_init(void)
 		init_gro_hash(&sd->backlog);
 		sd->backlog.poll = process_backlog;
 		sd->backlog.weight = weight_p;
+
+		if (net_page_pool_alloc(i))
+			goto out;
 	}
 

Review static int __init net_dev_init(void) · line 11798

Quoted 8 lines

(quoted diff) +3 -0

 	dev_boot_phase = 0;

@@ -11765,6 +11793,18 @@ static int __init net_dev_init(void)
 	WARN_ON(rc < 0);
 	rc = 0;
 out:
+	if (rc < 0) {
+		for_each_possible_cpu(i) {
+			struct page_pool *pp_ptr = this_cpu_read(page_pool);

this_cpu_read() -> per_cpu_ptr()?

Quoted 12 lines

(quoted diff) +9 -0

+
+			if (!pp_ptr)
+				continue;
+
+			page_pool_destroy(pp_ptr);
+			per_cpu(page_pool, i) = NULL;
+		}
+	}
+
 	return rc;
 }

Quoted 9 lines · Lorenzo Bianconi

Quoted 8 lines

#ifdef CONFIG_LOCKDEP
/*
* register_netdevice() inits txq->_xmit_lock and sets lockdep class

(quoted diff) +0 -0

@@ -11686,6 +11690,27 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Quoted 5 lines

Review static void __init net_dev_struct_check(void) · line 11693

Quoted 1 line

(quoted diff) +1 -0

+#define SD_PAGE_POOL_RING_SIZE	256

I might missed that if there is a reason we choose 256 here, do we need to use different value for differe page size, for 64K page size, it means we might need to reserve 16MB memory for each CPU.

honestly I have not spent time on it, most of the current page_pool users set pool_size to 256. Anyway, do you mean something like:

net/core/dev.c +0 -0

index f70fb6cad2b2..3934a3fc5c45 100644

net/core/dev.c +0 -0

 		.nid = NUMA_NO_NODE,
 	};
 	struct page_pool *pp_ptr;

Quoted 26 lines

Quoted 9 lines · Lorenzo Bianconi

Quoted 8 lines

#ifdef CONFIG_LOCKDEP
/*
* register_netdevice() inits txq->_xmit_lock and sets lockdep class

(quoted diff) +0 -0

@@ -11686,6 +11690,27 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Quoted 5 lines

Review static void __init net_dev_struct_check(void) · line 11693

Quoted 1 line

(quoted diff) +1 -0

+#define SD_PAGE_POOL_RING_SIZE	256

I might missed that if there is a reason we choose 256 here, do we need to use different value for differe page size, for 64K page size, it means we might need to reserve 16MB memory for each CPU.

honestly I have not spent time on it, most of the current page_pool users set pool_size to 256. Anyway, do you mean something like:

net/core/dev.c +0 -0

index f70fb6cad2b2..3934a3fc5c45 100644
@@ -11806,12 +11806,11 @@ static void __init net_dev_struct_check(void)
  *
  */
 

net/core/dev.c static void __init net_dev_struct_check(void) · line 11811

Quoted 4 lines

net/core/dev.c +0 -1

-#define SD_PAGE_POOL_RING_SIZE	256
 static int net_page_pool_alloc(int cpuid)
 {
 #if IS_ENABLED(CONFIG_PAGE_POOL)

Isn't better to have a config like CONFIG_PER_CPU_PAGE_POOL to enable this feature? and this config can be selected by whoever needs this feature?

net/core/dev.c static void __init net_dev_struct_check(void) · line 11813

Quoted 4 lines

net/core/dev.c +1 -1

 	struct page_pool_params page_pool_params = {
-		.pool_size = SD_PAGE_POOL_RING_SIZE,
+		.pool_size = PAGE_SIZE < SZ_64K ? 256 : 16,

What about other page size? like 16KB?
How about something like below:
PAGE_SIZE << get_order(PER_CPU_PAGE_POOL_MAX_SIZE)

Quoted 4 lines

net/core/dev.c +0 -0

 		.nid = NUMA_NO_NODE,
 	};
 	struct page_pool *pp_ptr;

Quoted 27 lines · Lorenzo Bianconi

Quoted 26 lines

Quoted 9 lines · Lorenzo Bianconi

Quoted 8 lines

#ifdef CONFIG_LOCKDEP
/*
* register_netdevice() inits txq->_xmit_lock and sets lockdep class

(quoted diff) +0 -0

@@ -11686,6 +11690,27 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Quoted 5 lines

Review static void __init net_dev_struct_check(void) · line 11693

Quoted 1 line

(quoted diff) +1 -0

+#define SD_PAGE_POOL_RING_SIZE	256

I might missed that if there is a reason we choose 256 here, do we need to use different value for differe page size, for 64K page size, it means we might need to reserve 16MB memory for each CPU.

honestly I have not spent time on it, most of the current page_pool users set pool_size to 256. Anyway, do you mean something like:

net/core/dev.c +0 -0

index f70fb6cad2b2..3934a3fc5c45 100644
@@ -11806,12 +11806,11 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Quoted 8 lines

net/core/dev.c static void __init net_dev_struct_check(void) · line 11811

Quoted 4 lines

net/core/dev.c +0 -1

-#define SD_PAGE_POOL_RING_SIZE	256
 static int net_page_pool_alloc(int cpuid)
 {
 #if IS_ENABLED(CONFIG_PAGE_POOL)

Isn't better to have a config like CONFIG_PER_CPU_PAGE_POOL to enable this feature? and this config can be selected by whoever needs this feature?

since it will be used for generic xdp (at least) I think this will be 99% enabled when we have bpf enabled, right?

Quoted 8 lines

net/core/dev.c static void __init net_dev_struct_check(void) · line 11813

Quoted 4 lines

net/core/dev.c +1 -1

 	struct page_pool_params page_pool_params = {
-		.pool_size = SD_PAGE_POOL_RING_SIZE,
+		.pool_size = PAGE_SIZE < SZ_64K ? 256 : 16,

What about other page size? like 16KB?
How about something like below:
PAGE_SIZE << get_order(PER_CPU_PAGE_POOL_MAX_SIZE)

since pool_size is the number of elements in the ptr_ring associated to the pool, assuming we want to consume PER_CPU_PAGE_POOL_MAX_SIZE for each cpu, something like:

PER_CPU_PAGE_POOL_MAX_SIZE / PAGE_SIZE

Regards,
Lorenzo

Quoted 6 lines

Quoted 5 lines

net/core/dev.c +0 -0

 		.nid = NUMA_NO_NODE,
 	};
 	struct page_pool *pp_ptr;

Quoted 48 lines

Quoted 27 lines · Lorenzo Bianconi

Quoted 26 lines

Quoted 9 lines · Lorenzo Bianconi

Quoted 8 lines

#ifdef CONFIG_LOCKDEP
/*
* register_netdevice() inits txq->_xmit_lock and sets lockdep class

(quoted diff) +0 -0

@@ -11686,6 +11690,27 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Quoted 5 lines

Review static void __init net_dev_struct_check(void) · line 11693

Quoted 1 line

(quoted diff) +1 -0

+#define SD_PAGE_POOL_RING_SIZE	256

I might missed that if there is a reason we choose 256 here, do we need to use different value for differe page size, for 64K page size, it means we might need to reserve 16MB memory for each CPU.

honestly I have not spent time on it, most of the current page_pool users set pool_size to 256. Anyway, do you mean something like:

net/core/dev.c +0 -0

index f70fb6cad2b2..3934a3fc5c45 100644
@@ -11806,12 +11806,11 @@ static void __init net_dev_struct_check(void)
  *
  */
 

Quoted 8 lines

net/core/dev.c static void __init net_dev_struct_check(void) · line 11811

Quoted 4 lines

net/core/dev.c +0 -1

-#define SD_PAGE_POOL_RING_SIZE	256
 static int net_page_pool_alloc(int cpuid)
 {
 #if IS_ENABLED(CONFIG_PAGE_POOL)

Isn't better to have a config like CONFIG_PER_CPU_PAGE_POOL to enable this feature? and this config can be selected by whoever needs this feature?

since it will be used for generic xdp (at least) I think this will be 99% enabled when we have bpf enabled, right?

Quoted 8 lines

net/core/dev.c static void __init net_dev_struct_check(void) · line 11813

Quoted 4 lines

net/core/dev.c +1 -1

 	struct page_pool_params page_pool_params = {
-		.pool_size = SD_PAGE_POOL_RING_SIZE,
+		.pool_size = PAGE_SIZE < SZ_64K ? 256 : 16,

What about other page size? like 16KB?
How about something like below:
PAGE_SIZE << get_order(PER_CPU_PAGE_POOL_MAX_SIZE)

Quoted 5 lines

since pool_size is the number of elements in the ptr_ring associated to the pool, assuming we want to consume PER_CPU_PAGE_POOL_MAX_SIZE for each cpu, something like:

PER_CPU_PAGE_POOL_MAX_SIZE / PAGE_SIZE

Quoted 3 lines

Regards,
Lorenzo

Discussing with Jesper and Toke, we agreed page_pool infrastructure will need a way to release memory when the system is under memory pressure, so we can defer this item to a subsequent series, what do you think?

Introduce generic percpu page_pools allocator.
Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct in order to recycle the page in the page_pool "hot" cache if napi_pp_put_page() is running on the same cpu.
This is a preliminary patch to add xdp multi-buff support for xdp running in generic mode.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/page_pool/types.h |  3 +++
 net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
 net/core/page_pool.c          | 23 ++++++++++++++++----
 net/core/skbuff.c             |  5 +++--
 4 files changed, 65 insertions(+), 6 deletions(-)

include/net/page_pool/types.h +3 -0

index 76481c465375..3828396ae60c 100644
@@ -128,6 +128,7 @@ struct page_pool_stats {
 struct page_pool {
 	struct page_pool_params_fast p;
 
+	int cpuid;
 	bool has_init_callback;
 
 	long frag_users;

@@ -203,6 +204,8 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
 struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
 				  unsigned int size, gfp_t gfp);
 struct page_pool *page_pool_create(const struct page_pool_params *params);
+struct page_pool *page_pool_create_percpu(const struct page_pool_params *params,
+					  int cpuid);
 
 struct xdp_mem_info;
 

net/core/dev.c +2 -0

index cb2dab0feee0..bf9ec740b09a 100644
@@ -153,6 +153,8 @@
 #include <linux/prandom.h>
 #include <linux/once_lite.h>
 #include <net/netdev_rx_queue.h>
+#include <net/page_pool/types.h>
+#include <net/page_pool/helpers.h>
 
 #include "dev.h"
 #include "net-sysfs.h"

Quoted 53 lines · Lorenzo Bianconi

Introduce generic percpu page_pools allocator.
Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct in order to recycle the page in the page_pool "hot" cache if napi_pp_put_page() is running on the same cpu.
This is a preliminary patch to add xdp multi-buff support for xdp running in generic mode.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/page_pool/types.h |  3 +++
 net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
 net/core/page_pool.c          | 23 ++++++++++++++++----
 net/core/skbuff.c             |  5 +++--
 4 files changed, 65 insertions(+), 6 deletions(-)

include/net/page_pool/types.h +3 -0

index 76481c465375..3828396ae60c 100644
@@ -128,6 +128,7 @@ struct page_pool_stats {
 struct page_pool {
 	struct page_pool_params_fast p;
 
+	int cpuid;
 	bool has_init_callback;
 
 	long frag_users;

@@ -203,6 +204,8 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
 struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
 				  unsigned int size, gfp_t gfp);
 struct page_pool *page_pool_create(const struct page_pool_params *params);
+struct page_pool *page_pool_create_percpu(const struct page_pool_params *params,
+					  int cpuid);
 
 struct xdp_mem_info;
 

net/core/dev.c +2 -0

index cb2dab0feee0..bf9ec740b09a 100644
@@ -153,6 +153,8 @@
 #include <linux/prandom.h>
 #include <linux/once_lite.h>
 #include <net/netdev_rx_queue.h>
+#include <net/page_pool/types.h>
+#include <net/page_pool/helpers.h>
 
 #include "dev.h"
 #include "net-sysfs.h"

@@ -442,6 +444,8 @@ static RAW_NOTIFIER_HEAD(netdev_chain);
 DEFINE_PER_CPU_ALIGNED(struct softnet_data, softnet_data);
 EXPORT_PER_CPU_SYMBOL(softnet_data);
 

net/core/dev.c static RAW_NOTIFIER_HEAD(netdev_chain); · line 447

Quoted 1 line

net/core/dev.c +1 -0

+DEFINE_PER_CPU_ALIGNED(struct page_pool *, page_pool);

I think we should come up with a better name than just "page_pool" for this global var. In the code below it looks like it's a local variable that's being referenced. Maybe "global_page_pool" or "system_page_pool" or something along those lines?

-Toke

Quoted 55 lines

Quoted 53 lines · Lorenzo Bianconi

Introduce generic percpu page_pools allocator.
Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct in order to recycle the page in the page_pool "hot" cache if napi_pp_put_page() is running on the same cpu.
This is a preliminary patch to add xdp multi-buff support for xdp running in generic mode.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/page_pool/types.h |  3 +++
 net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
 net/core/page_pool.c          | 23 ++++++++++++++++----
 net/core/skbuff.c             |  5 +++--
 4 files changed, 65 insertions(+), 6 deletions(-)

include/net/page_pool/types.h +3 -0

index 76481c465375..3828396ae60c 100644
@@ -128,6 +128,7 @@ struct page_pool_stats {
 struct page_pool {
 	struct page_pool_params_fast p;
 
+	int cpuid;
 	bool has_init_callback;
 
 	long frag_users;

@@ -203,6 +204,8 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
 struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
 				  unsigned int size, gfp_t gfp);
 struct page_pool *page_pool_create(const struct page_pool_params *params);
+struct page_pool *page_pool_create_percpu(const struct page_pool_params *params,
+					  int cpuid);
 
 struct xdp_mem_info;
 

net/core/dev.c +2 -0

index cb2dab0feee0..bf9ec740b09a 100644
@@ -153,6 +153,8 @@
 #include <linux/prandom.h>
 #include <linux/once_lite.h>
 #include <net/netdev_rx_queue.h>
+#include <net/page_pool/types.h>
+#include <net/page_pool/helpers.h>
 
 #include "dev.h"
 #include "net-sysfs.h"

@@ -442,6 +444,8 @@ static RAW_NOTIFIER_HEAD(netdev_chain);
 DEFINE_PER_CPU_ALIGNED(struct softnet_data, softnet_data);
 EXPORT_PER_CPU_SYMBOL(softnet_data);
 

Quoted 6 lines

net/core/dev.c static RAW_NOTIFIER_HEAD(netdev_chain); · line 447

Quoted 1 line

net/core/dev.c +1 -0

+DEFINE_PER_CPU_ALIGNED(struct page_pool *, page_pool);

I think we should come up with a better name than just "page_pool" for this global var. In the code below it looks like it's a local variable that's being referenced. Maybe "global_page_pool" or "system_page_pool" or something along those lines?

ack, I will fix it. system_page_pool seems better, agree?

Quoted 62 lines · Lorenzo Bianconi

Quoted 61 lines

Quoted 50 lines · Lorenzo Bianconi

Introduce generic percpu page_pools allocator.
Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct in order to recycle the page in the page_pool "hot" cache if napi_pp_put_page() is running on the same cpu.
This is a preliminary patch to add xdp multi-buff support for xdp running in generic mode.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
  include/net/page_pool/types.h |  3 +++
  net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
  net/core/page_pool.c          | 23 ++++++++++++++++----
  net/core/skbuff.c             |  5 +++--
  4 files changed, 65 insertions(+), 6 deletions(-)

include/net/page_pool/types.h +3 -0

index 76481c465375..3828396ae60c 100644
@@ -128,6 +128,7 @@ struct page_pool_stats {
  struct page_pool {
  	struct page_pool_params_fast p;
 
+	int cpuid;
  	bool has_init_callback;
 
  	long frag_users;

@@ -203,6 +204,8 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
  struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
  				  unsigned int size, gfp_t gfp);
  struct page_pool *page_pool_create(const struct page_pool_params *params);
+struct page_pool *page_pool_create_percpu(const struct page_pool_params *params,
+					  int cpuid);
 
  struct xdp_mem_info;
 

net/core/dev.c +2 -0

index cb2dab0feee0..bf9ec740b09a 100644
@@ -153,6 +153,8 @@
  #include <linux/prandom.h>
  #include <linux/once_lite.h>
  #include <net/netdev_rx_queue.h>
+#include <net/page_pool/types.h>
+#include <net/page_pool/helpers.h>
 
  #include "dev.h"
  #include "net-sysfs.h"

net/core/dev.c static RAW_NOTIFIER_HEAD(netdev_chain); · line 447

Quoted 4 lines

net/core/dev.c +1 -0

@@ -442,6 +444,8 @@ static RAW_NOTIFIER_HEAD(netdev_chain);
  DEFINE_PER_CPU_ALIGNED(struct softnet_data, softnet_data);
  EXPORT_PER_CPU_SYMBOL(softnet_data);
 
+DEFINE_PER_CPU_ALIGNED(struct page_pool *, page_pool);

I think we should come up with a better name than just "page_pool" for this global var. In the code below it looks like it's a local variable that's being referenced. Maybe "global_page_pool" or "system_page_pool" or something along those lines?

Quoted 1 line

ack, I will fix it. system_page_pool seems better, agree?

Yeah, agreed :)

Introduce generic percpu page_pools allocator.
Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct in order to recycle the page in the page_pool "hot" cache if napi_pp_put_page() is running on the same cpu.
This is a preliminary patch to add xdp multi-buff support for xdp running in generic mode.

Signed-off-by Lorenzo Bianconi<[email protected]>

---
  include/net/page_pool/types.h |  3 +++
  net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
  net/core/page_pool.c          | 23 ++++++++++++++++----
  net/core/skbuff.c             |  5 +++--
  4 files changed, 65 insertions(+), 6 deletions(-)

net/core/dev.c +0 -0

index cb2dab0feee0..bf9ec740b09a 100644

net/core/dev.c +34 -0

+{
+#if IS_ENABLED(CONFIG_PAGE_POOL)
+	struct page_pool_params page_pool_params = {
+		.pool_size = SD_PAGE_POOL_RING_SIZE,
+		.nid = NUMA_NO_NODE,
+	};
+	struct page_pool *pp_ptr;
+
+	pp_ptr = page_pool_create_percpu(&page_pool_params, cpuid);
+	if (IS_ERR(pp_ptr)) {
+		pp_ptr = NULL;
+		return -ENOMEM;
+	}
+
+	per_cpu(page_pool, cpuid) = pp_ptr;
+#endif
+	return 0;
+}
+
  /*
   *       This is called single threaded during boot, so no need
   *       to take the rtnl semaphore.

@@ -11738,6 +11763,9 @@ static int __init net_dev_init(void)
  		init_gro_hash(&sd->backlog);
  		sd->backlog.poll = process_backlog;
  		sd->backlog.weight = weight_p;
+
+		if (net_page_pool_alloc(i))
+			goto out;
  	}
 
  	dev_boot_phase = 0;

@@ -11765,6 +11793,18 @@ static int __init net_dev_init(void)
  	WARN_ON(rc < 0);
  	rc = 0;
  out:
+	if (rc < 0) {
+		for_each_possible_cpu(i) {
+			struct page_pool *pp_ptr = this_cpu_read(page_pool);
+
+			if (!pp_ptr)
+				continue;
+
+			page_pool_destroy(pp_ptr);
+			per_cpu(page_pool, i) = NULL;
+		}
+	}
+
  	return rc;
  }
 

Quoted 15 lines

Introduce generic percpu page_pools allocator.
Moreover add page_pool_create_percpu() and cpuid filed in page_pool struct in order to recycle the page in the page_pool "hot" cache if napi_pp_put_page() is running on the same cpu.
This is a preliminary patch to add xdp multi-buff support for xdp running in generic mode.

Signed-off-by Lorenzo Bianconi<[email protected]>

---
  include/net/page_pool/types.h |  3 +++
  net/core/dev.c                | 40 +++++++++++++++++++++++++++++++++++
  net/core/page_pool.c          | 23 ++++++++++++++++----
  net/core/skbuff.c             |  5 +++--
  4 files changed, 65 insertions(+), 6 deletions(-)

[...]

Quoted 4 lines

net/core/dev.c +0 -0

index cb2dab0feee0..bf9ec740b09a 100644

[...]

net/core/dev.c static void __init net_dev_struct_check(void) · line 11693

Quoted 5 lines

net/core/dev.c +2 -0

@@ -11686,6 +11690,27 @@ static void __init net_dev_struct_check(void)
   *
   */
+#define SD_PAGE_POOL_RING_SIZE	256
+static int net_page_pool_alloc(int cpuid)

I don't like the name net_page_pool_alloc().
It uses the page_pool_create APIs.

Let us renamed to net_page_pool_create() ?

Quoted 51 lines

net/core/dev.c +34 -0

+{
+#if IS_ENABLED(CONFIG_PAGE_POOL)
+	struct page_pool_params page_pool_params = {
+		.pool_size = SD_PAGE_POOL_RING_SIZE,
+		.nid = NUMA_NO_NODE,
+	};
+	struct page_pool *pp_ptr;
+
+	pp_ptr = page_pool_create_percpu(&page_pool_params, cpuid);
+	if (IS_ERR(pp_ptr)) {
+		pp_ptr = NULL;
+		return -ENOMEM;
+	}
+
+	per_cpu(page_pool, cpuid) = pp_ptr;
+#endif
+	return 0;
+}
+
  /*
   *       This is called single threaded during boot, so no need
   *       to take the rtnl semaphore.

@@ -11738,6 +11763,9 @@ static int __init net_dev_init(void)
  		init_gro_hash(&sd->backlog);
  		sd->backlog.poll = process_backlog;
  		sd->backlog.weight = weight_p;
+
+		if (net_page_pool_alloc(i))
+			goto out;
  	}
  	dev_boot_phase = 0;

@@ -11765,6 +11793,18 @@ static int __init net_dev_init(void)
  	WARN_ON(rc < 0);
  	rc = 0;
  out:
+	if (rc < 0) {
+		for_each_possible_cpu(i) {
+			struct page_pool *pp_ptr = this_cpu_read(page_pool);
+
+			if (!pp_ptr)
+				continue;
+
+			page_pool_destroy(pp_ptr);
+			per_cpu(page_pool, i) = NULL;
+		}
+	}
+
  	return rc;
  }

Rely on skb pointer reference instead of the skb pointer in do_xdp_generic and netif_receive_generic_xdp routine signatures. This is a preliminary patch to add multi-buff support for xdp running in generic mode.

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

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/tun.c         |  4 ++--
 include/linux/netdevice.h |  2 +-
 net/core/dev.c            | 16 +++++++++-------
 3 files changed, 12 insertions(+), 10 deletions(-)

drivers/net/tun.c +2 -2

index 4a4f8c8e79fa..5bd98bdaddf2 100644
@@ -1927,7 +1927,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
                rcu_read_lock();
                xdp_prog = rcu_dereference(tun->xdp_prog);
                if (xdp_prog) {
-                       ret = do_xdp_generic(xdp_prog, skb);
+                       ret = do_xdp_generic(xdp_prog, &skb);
                        if (ret != XDP_PASS) {
                                rcu_read_unlock();
                                local_bh_enable();

@@ -2517,7 +2517,7 @@ static int tun_xdp_one(struct tun_struct *tun,
        skb_record_rx_queue(skb, tfile->queue_index);
 
        if (skb_xdp) {
-               ret = do_xdp_generic(xdp_prog, skb);
+               ret = do_xdp_generic(xdp_prog, &skb);
                if (ret != XDP_PASS) {
                        ret = 0;
                        goto out;

include/linux/netdevice.h +1 -1

index 118c40258d07..7eee99a58200 100644
@@ -3958,7 +3958,7 @@ static inline void dev_consume_skb_any(struct sk_buff *skb)
 u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
                             struct bpf_prog *xdp_prog);
 void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog);
-int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb);
+int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff **pskb);
 int netif_rx(struct sk_buff *skb);
 int __netif_rx(struct sk_buff *skb);
 

net/core/dev.c +9 -8

index bf9ec740b09a..960f39ac5e33 100644
@@ -4924,10 +4924,11 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
        return act;
 }
 
-static u32 netif_receive_generic_xdp(struct sk_buff *skb,
+static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
                                     struct xdp_buff *xdp,
                                     struct bpf_prog *xdp_prog)
 {
+       struct sk_buff *skb = *pskb;
        u32 act = XDP_DROP;
 
        /* Reinjected packets coming from act_mirred or similar should

@@ -5008,24 +5009,24 @@ void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog)
 
 static DEFINE_STATIC_KEY_FALSE(generic_xdp_needed_key);
 
-int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
+int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff **pskb)
 {
        if (xdp_prog) {
                struct xdp_buff xdp;
                u32 act;
                int err;
 
-               act = netif_receive_generic_xdp(skb, &xdp, xdp_prog);
+               act = netif_receive_generic_xdp(pskb, &xdp, xdp_prog);
                if (act != XDP_PASS) {
                        switch (act) {
                        case XDP_REDIRECT:
-                               err = xdp_do_generic_redirect(skb->dev, skb,
+                               err = xdp_do_generic_redirect((*pskb)->dev, *pskb,
                                                              &xdp, xdp_prog);
                                if (err)
                                        goto out_redir;
                                break;
                        case XDP_TX:
-                               generic_xdp_tx(skb, xdp_prog);
+                               generic_xdp_tx(*pskb, xdp_prog);
                                break;
                        }
                        return XDP_DROP;

@@ -5033,7 +5034,7 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
        }
        return XDP_PASS;
 out_redir:
-       kfree_skb_reason(skb, SKB_DROP_REASON_XDP);
+       kfree_skb_reason(*pskb, SKB_DROP_REASON_XDP);
        return XDP_DROP;
 }
 EXPORT_SYMBOL_GPL(do_xdp_generic);

@@ -5356,7 +5357,8 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
                int ret2;
 
                migrate_disable();
-               ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog), skb);
+               ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog),
+                                     &skb);
                migrate_enable();
 
                if (ret2 != XDP_PASS) {
--

2.43.0

Hi Lorenzo,

Quoted 4 lines · Lorenzo Bianconi

Rely on skb pointer reference instead of the skb pointer in do_xdp_generic and netif_receive_generic_xdp routine signatures. This is a preliminary patch to add multi-buff support for xdp running in generic mode.

The patch looks fine, but can we tweak the commit message explaining in more detail why this is needed?

Thanks
/Ilias

Quoted 112 lines

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

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/tun.c         |  4 ++--
 include/linux/netdevice.h |  2 +-
 net/core/dev.c            | 16 +++++++++-------
 3 files changed, 12 insertions(+), 10 deletions(-)

drivers/net/tun.c +2 -2

index 4a4f8c8e79fa..5bd98bdaddf2 100644
@@ -1927,7 +1927,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
                rcu_read_lock();
                xdp_prog = rcu_dereference(tun->xdp_prog);
                if (xdp_prog) {
-                       ret = do_xdp_generic(xdp_prog, skb);
+                       ret = do_xdp_generic(xdp_prog, &skb);
                        if (ret != XDP_PASS) {
                                rcu_read_unlock();
                                local_bh_enable();

@@ -2517,7 +2517,7 @@ static int tun_xdp_one(struct tun_struct *tun,
        skb_record_rx_queue(skb, tfile->queue_index);
 
        if (skb_xdp) {
-               ret = do_xdp_generic(xdp_prog, skb);
+               ret = do_xdp_generic(xdp_prog, &skb);
                if (ret != XDP_PASS) {
                        ret = 0;
                        goto out;

include/linux/netdevice.h +1 -1

index 118c40258d07..7eee99a58200 100644
@@ -3958,7 +3958,7 @@ static inline void dev_consume_skb_any(struct sk_buff *skb)
 u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
                             struct bpf_prog *xdp_prog);
 void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog);
-int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb);
+int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff **pskb);
 int netif_rx(struct sk_buff *skb);
 int __netif_rx(struct sk_buff *skb);
 

net/core/dev.c +9 -8

index bf9ec740b09a..960f39ac5e33 100644
@@ -4924,10 +4924,11 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
        return act;
 }
 
-static u32 netif_receive_generic_xdp(struct sk_buff *skb,
+static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
                                     struct xdp_buff *xdp,
                                     struct bpf_prog *xdp_prog)
 {
+       struct sk_buff *skb = *pskb;
        u32 act = XDP_DROP;
 
        /* Reinjected packets coming from act_mirred or similar should

@@ -5008,24 +5009,24 @@ void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog)
 
 static DEFINE_STATIC_KEY_FALSE(generic_xdp_needed_key);
 
-int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
+int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff **pskb)
 {
        if (xdp_prog) {
                struct xdp_buff xdp;
                u32 act;
                int err;
 
-               act = netif_receive_generic_xdp(skb, &xdp, xdp_prog);
+               act = netif_receive_generic_xdp(pskb, &xdp, xdp_prog);
                if (act != XDP_PASS) {
                        switch (act) {
                        case XDP_REDIRECT:
-                               err = xdp_do_generic_redirect(skb->dev, skb,
+                               err = xdp_do_generic_redirect((*pskb)->dev, *pskb,
                                                              &xdp, xdp_prog);
                                if (err)
                                        goto out_redir;
                                break;
                        case XDP_TX:
-                               generic_xdp_tx(skb, xdp_prog);
+                               generic_xdp_tx(*pskb, xdp_prog);
                                break;
                        }
                        return XDP_DROP;

@@ -5033,7 +5034,7 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
        }
        return XDP_PASS;
 out_redir:
-       kfree_skb_reason(skb, SKB_DROP_REASON_XDP);
+       kfree_skb_reason(*pskb, SKB_DROP_REASON_XDP);
        return XDP_DROP;
 }
 EXPORT_SYMBOL_GPL(do_xdp_generic);

@@ -5356,7 +5357,8 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
                int ret2;
 
                migrate_disable();
-               ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog), skb);
+               ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog),
+                                     &skb);
                migrate_enable();
 
                if (ret2 != XDP_PASS) {
--

2.43.0

net/core/dev.c u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp, · line 4944

Quoted 4 lines

net/core/dev.c +4 -0

+#if IS_ENABLED(CONFIG_PAGE_POOL)
+static int
+netif_skb_segment_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
+			  struct bpf_prog *prog)

nit: doesn't look all that related to a netif, I'd put it in skbuff.c

Quoted 1 line

nit: doesn't look all that related to a netif, I'd put it in skbuff.c

ack, fine. skb_segment_for_xdp() in this case?

Quoted 3 lines

Quoted 1 line

nit: doesn't look all that related to a netif, I'd put it in skbuff.c

ack, fine. skb_segment_for_xdp() in this case?

I think the closest thing we have now is skb_cow_data(), so how about skb_cow_data_pp() or skb_cow_fragged() or skb_cow_something? :)

I'm on the fence whether we should split the XDP-ness out. I mean the only two xdp-related things are the headroom and check for xdp_has_frags, so we could also:

skb_cow_data_pp(struct page_pool *pool, struct sk_buff **pskb,
unsigned int headroom)

{
	...
}

skb_cow_data_xdp(struct page_pool *pool, struct sk_buff **pskb,
struct bpf_prog *prog)

{
	if (!prog->aux->xdp_has_frags)
		return -EINVAL;

	return skb_cow_data_pp(pool, pskb, XDP_PACKET_HEADROOM);
}

I think it'd increase the chances of reuse. But that's speculative so I'll let you decide if you prefer that or to keep it simple.

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Quoted 2 lines

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Is there any reason why we do not need those kind stats for per cpu page_pool?

Quoted 7 lines · Lorenzo Bianconi

Quoted 2 lines

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Is there any reason why we do not need those kind stats for per cpu page_pool?

IIRC discussing with Jakub, we decided to not support them since the pool is not associated to any net_device in this case.

Regards,
Lorenzo

Quoted 4 lines

Quoted 4 lines · Lorenzo Bianconi

Quoted 2 lines

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Quoted 6 lines

Quoted 3 lines

Is there any reason why we do not need those kind stats for per cpu page_pool?

IIRC discussing with Jakub, we decided to not support them since the pool is not associated to any net_device in this case.

It seems what jakub suggested is to 'extend netlink to dump unbound page pools'?

Quoted 5 lines

Regards,
Lorenzo

Quoted 5 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines · Lorenzo Bianconi

Quoted 2 lines

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Is there any reason why we do not need those kind stats for per cpu page_pool?

IIRC discussing with Jakub, we decided to not support them since the pool is not associated to any net_device in this case.

It seems what jakub suggested is to 'extend netlink to dump unbound page pools'?

I do not have a strong opinion about it (since we do not have any use-case for it at the moment).
In the case we want to support stats for per-cpu page_pools, I think we should not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field in page_pool struct since otherwise we will endup with ncpu^2 copies, right? Do we want to support it now?

@Jakub, Jesper: what do you guys think?

Quoted 21 lines

Quoted 5 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines · Lorenzo Bianconi

Quoted 2 lines

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Is there any reason why we do not need those kind stats for per cpu page_pool?

IIRC discussing with Jakub, we decided to not support them since the pool is not associated to any net_device in this case.

It seems what jakub suggested is to 'extend netlink to dump unbound page pools'?

I do not have a strong opinion about it (since we do not have any use-case for it at the moment).
In the case we want to support stats for per-cpu page_pools, I think we should not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field in page_pool struct since otherwise we will endup with ncpu^2 copies, right? Do we want to support it now?

Quoted 2 lines

@Jakub, Jesper: what do you guys think?

I do see an need for being able to access page_pool stats for all page_pool's in the system.
And I do like Jakub's netlink based stats.

--Jesper
(p.s. I'm debugging some production issues with page_pool and broadcom bnxt_en driver).

Quoted 24 lines · Lorenzo Bianconi

Quoted 21 lines

Quoted 5 lines · Lorenzo Bianconi

Quoted 4 lines

Quoted 4 lines · Lorenzo Bianconi

Quoted 2 lines

Move page_pool stats allocation in page_pool_create routine and get rid of it for percpu page_pools.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Is there any reason why we do not need those kind stats for per cpu page_pool?

IIRC discussing with Jakub, we decided to not support them since the pool is not associated to any net_device in this case.

It seems what jakub suggested is to 'extend netlink to dump unbound page pools'?

I do not have a strong opinion about it (since we do not have any use-case for it at the moment).
In the case we want to support stats for per-cpu page_pools, I think we should not create a per-cpu recycle_stats pointer and add a page_pool_recycle_stats field in page_pool struct since otherwise we will endup with ncpu^2 copies, right? Do we want to support it now?

Quoted 7 lines

Quoted 2 lines

@Jakub, Jesper: what do you guys think?

I do see an need for being able to access page_pool stats for all page_pool's in the system.
And I do like Jakub's netlink based stats.

ack from my side if you have some use-cases in mind.
Some questions below:
- can we assume ethtool will be used to report stats just for 'global' page_pool (not per-cpu page_pool)?
- can we assume netlink/yaml will be used to report per-cpu page_pool stats?

I think in the current series we can fix the accounting part (in particular avoiding memory wasting) and then we will figure out how to report percpu page_pool stats through netlink/yaml. Agree?

Quoted 6 lines

ack from my side if you have some use-cases in mind.
Some questions below:
- can we assume ethtool will be used to report stats just for 'global' page_pool (not per-cpu page_pool)?
- can we assume netlink/yaml will be used to report per-cpu page_pool stats?

Quoted 3 lines

I think in the current series we can fix the accounting part (in particular avoiding memory wasting) and then we will figure out how to report percpu page_pool stats through netlink/yaml. Agree?

Deferring the export API to a separate series after this is merged is fine with me.

In which case the *gathering* of statistics could also be deferred (it's not really useful if it can't be exported).

Quoted 6 lines

Quoted 6 lines

ack from my side if you have some use-cases in mind.
Some questions below:
- can we assume ethtool will be used to report stats just for 'global' page_pool (not per-cpu page_pool)?
- can we assume netlink/yaml will be used to report per-cpu page_pool stats?

Quoted 6 lines

Quoted 3 lines

I think in the current series we can fix the accounting part (in particular avoiding memory wasting) and then we will figure out how to report percpu page_pool stats through netlink/yaml. Agree?

Deferring the export API to a separate series after this is merged is fine with me.

+1

Quoted 2 lines

In which case the *gathering* of statistics could also be deferred (it's not really useful if it can't be exported).

What do you mean by "gather" here? If we plan to expose them later on I reckon there's no point having this patch which actively optimizes them away, no? IOW we should just drop this patch from v7?

Introduce multi-buffer support for xdp running in generic mode not always linearizing the skb in netif_receive_generic_xdp routine. Introduce page_pool in softnet_data structure

Quoted 3 lines · Lorenzo Bianconi

Introduce multi-buffer support for xdp running in generic mode not always linearizing the skb in netif_receive_generic_xdp routine. Introduce page_pool in softnet_data structure

This last line is not accurate anymore... :)

-Toke