Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function
@ 2019-12-10 11:26 Duncan Roe
  2019-12-10 11:26 ` [PATCH libnetfilter_queue 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Duncan Roe @ 2019-12-10 11:26 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

pktb_usebuf() is a copy of pktb_alloc() with the first part modified to use
a supplied buffer rather than calloc() one. I thought this would give a
measurable performance boost and it does.
All the code after the memset() call is common to pktb_alloc(). If you like,
I can submit a v2 with this code in a static function called by both.

Duncan Roe (1):
  src: Add alternative function to pktb_alloc to avoid malloc / free
    overhead

 include/libnetfilter_queue/pktbuff.h |  2 +
 src/extra/pktbuff.c                  | 82 ++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

--
2.14.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH libnetfilter_queue 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead
  2019-12-10 11:26 [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
@ 2019-12-10 11:26 ` Duncan Roe
  2019-12-22  2:09 ` [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Duncan Roe @ 2019-12-10 11:26 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

New function pktb_usebuf() works like pktb_alloc() except it has 2 extra
arguments, being a user-supplied buffer and its length.
Performance testing is ongoing, so the 5% improvement claimed in the
documentation is conservative.

Updated:

 include/libnetfilter_queue/pktbuff.h: Add pktb_usebuf() prototype

 src/extra/pktbuff.c: Implement pktb_usebuf()

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 include/libnetfilter_queue/pktbuff.h |  2 +
 src/extra/pktbuff.c                  | 82 ++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index b15ee1e..ec75727 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -4,6 +4,8 @@
 struct pkt_buff;
 
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
+struct pkt_buff *pktb_usebuf(int family, void *data, size_t len, size_t extra,
+			     void *buf, size_t bufsize);
 void pktb_free(struct pkt_buff *pktb);
 
 uint8_t *pktb_data(struct pkt_buff *pktb);
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index c4f3da3..774e4ab 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -96,6 +96,88 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	return pktb;
 }
 
+/**
+ * pktb_usebuf - make a packet buffer from an existing buffer
+ * \param family Indicate what family. Currently supported families are
+ * AF_BRIDGE, AF_INET & AF_INET6.
+ * \param data Pointer to packet data
+ * \param len Packet length
+ * \param extra Extra memory in the tail to be allocated (for mangling)
+ * \param buf Existing buffer to use
+ * \param bufsize Size of _buf_
+ *
+ * This function builds a userspace packet buffer inside a supplied buffer.
+ * The packet buffer contains the packet data and some extra memory room in the
+ * tail (if requested).
+ *
+ * This function uses less CPU cycles than pktb_alloc() + pktb_free().
+ * Users can expect a decrease in overall CPU time in the order of 5%.
+ *
+ * \return Pointer to a userspace packet buffer aligned within _buf_ or NULL on
+ * failure.
+ * \par Errors
+ * __ENOMEM__ _bufsize_ is too small to accomodate _len_
+ * \n
+ * __EPROTONOSUPPORT__ _family_ was __AF_BRIDGE__ and this is not an IP packet
+ * (v4 or v6)
+ * \warning Do __not__ call pktb_free() on the returned pointer
+ */
+EXPORT_SYMBOL
+struct pkt_buff *pktb_usebuf(int family, void *data, size_t len, size_t extra,
+			     void *buf, size_t bufsize)
+{
+	struct pkt_buff *pktb;
+	void *pkt_data;
+
+	/* Better make sure alignment is correct. */
+	size_t extra2 =
+		(size_t)buf & 0x7 ? (~((size_t)buf & 0x7) & 0x7) + 1 : 0;
+
+	pktb = buf + extra2;
+	if (extra2 + sizeof(struct pkt_buff) + len + extra > bufsize)
+	{
+		errno = ENOMEM;
+		return NULL;
+	}
+	memset(pktb, 0, sizeof(struct pkt_buff) + len + extra);
+
+	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
+	memcpy(pkt_data, data, len);
+
+	pktb->len = len;
+	pktb->data_len = len + extra;
+
+	pktb->head = pkt_data;
+	pktb->data = pkt_data;
+	pktb->tail = pktb->head + len;
+
+	switch(family) {
+	case AF_INET:
+	case AF_INET6:
+		pktb->network_header = pktb->data;
+		break;
+	case AF_BRIDGE: {
+		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
+
+		pktb->mac_header = pktb->data;
+
+		switch(ethhdr->h_proto) {
+		case ETH_P_IP:
+		case ETH_P_IPV6:
+			pktb->network_header = pktb->data + ETH_HLEN;
+			break;
+		default:
+			/* This protocol is unsupported. */
+			errno = EPROTONOSUPPORT;
+			free(pktb);
+			return NULL;
+		}
+		break;
+	}
+	}
+	return pktb;
+}
+
 /**
  * pktb_data - get pointer to network packet
  * \param pktb Pointer to userspace packet buffer
-- 
2.14.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function
  2019-12-10 11:26 [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
  2019-12-10 11:26 ` [PATCH libnetfilter_queue 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe
@ 2019-12-22  2:09 ` Duncan Roe
  2020-01-03  2:47 ` Duncan Roe
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Duncan Roe @ 2019-12-22  2:09 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Development

Hi Pablo,

On Tue, Dec 10, 2019 at 10:26:33PM +1100, Duncan Roe wrote:
> pktb_usebuf() is a copy of pktb_alloc() with the first part modified to use
> a supplied buffer rather than calloc() one. I thought this would give a
> measurable performance boost and it does.
> All the code after the memset() call is common to pktb_alloc(). If you like,
> I can submit a v2 with this code in a static function called by both.
>
> Duncan Roe (1):
>   src: Add alternative function to pktb_alloc to avoid malloc / free
>     overhead
>
>  include/libnetfilter_queue/pktbuff.h |  2 +
>  src/extra/pktbuff.c                  | 82 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
>
> --
> 2.14.5
>
After a day or so of testing, it appears pktb_usebuf() saves 6% overall CPU over
pktb_alloc() in a program similar to libnetfilter_queue/examples/nf-queue.c (but
not printing every packet). The variance is quite high, so plus or minus 1%. Can
supply more details of testing methodology if you want them.

Overall I would say 6% is a worthwhile saving.

What about it?

Cheers ... Duncan.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function
  2019-12-10 11:26 [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
  2019-12-10 11:26 ` [PATCH libnetfilter_queue 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe
  2019-12-22  2:09 ` [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
@ 2020-01-03  2:47 ` Duncan Roe
  2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 0/1] New pktb_make() function Duncan Roe
  2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe
  4 siblings, 0 replies; 9+ messages in thread
From: Duncan Roe @ 2020-01-03  2:47 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Hi Pablo,

Do you think pktb_make() might be a better name for this new function?

I noticed an errant free() call after "errno = EPROTONOSUPPORT" so there will
have to be a v2 anyway.

I could merge the (now almost-) identical second halves of pktb_alloc and
pktb_make into a static function to avoid the duplicate code maintenance
overhead. Would you prefer that?

Cheers ... Duncan.

On Tue, Dec 10, 2019 at 10:26:33PM +1100, Duncan Roe wrote:
> pktb_usebuf() is a copy of pktb_alloc() with the first part modified to use
> a supplied buffer rather than calloc() one. I thought this would give a
> measurable performance boost and it does.
> All the code after the memset() call is common to pktb_alloc(). If you like,
> I can submit a v2 with this code in a static function called by both.
>
> Duncan Roe (1):
>   src: Add alternative function to pktb_alloc to avoid malloc / free
>     overhead
>
>  include/libnetfilter_queue/pktbuff.h |  2 +
>  src/extra/pktbuff.c                  | 82 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
>
> --
> 2.14.5
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH libnetfilter_queue v2 0/1] New pktb_make() function
  2019-12-10 11:26 [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
                   ` (2 preceding siblings ...)
  2020-01-03  2:47 ` Duncan Roe
@ 2020-01-06  3:17 ` Duncan Roe
  2020-01-08 22:53   ` Pablo Neira Ayuso
  2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe
  4 siblings, 1 reply; 9+ messages in thread
From: Duncan Roe @ 2020-01-06  3:17 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

This patch offers a faster alternative / replacement function to pktb_alloc().

pktb_make() is a copy of the first part of pktb_alloc() modified to use a
supplied buffer rather than calloc() one. It then calls the second part of
pktb_alloc() which is modified to be a static function.

Can't think of a use case where one would choose to use pktb_alloc over
pktb_make.
In a furure documentation update, might relegate pktb_alloc and pktb_free to
"other functions".

Duncan Roe (1):
  src: Add alternative function to pktb_alloc to avoid malloc / free
    overhead

 include/libnetfilter_queue/pktbuff.h |  1 +
 src/extra/pktbuff.c                  | 60 ++++++++++++++++++++++++++++++++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

v2: - Function name changed to pktb_make (was pktb_usebuf)
    - Cvt 2nd 1/2 of pktb_malloc into a static function common to pktb_make

-- 
2.14.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH libnetfilter_queue v2 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead
  2019-12-10 11:26 [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
                   ` (3 preceding siblings ...)
  2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 0/1] New pktb_make() function Duncan Roe
@ 2020-01-06  3:17 ` Duncan Roe
  4 siblings, 0 replies; 9+ messages in thread
From: Duncan Roe @ 2020-01-06  3:17 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

New function pktb_make() works like pktb_alloc() except it has 2 extra
arguments, being a user-supplied buffer and its length.
Performance testing using a program derived from examples/nf_queue.c shows
6% improvement, so the 5% improvement claimed in the documentation is
conservative.

Updated:

 include/libnetfilter_queue/pktbuff.h: Add pktb_make() prototype

 src/extra/pktbuff.c: Implement pktb_make()

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 include/libnetfilter_queue/pktbuff.h |  1 +
 src/extra/pktbuff.c                  | 60 ++++++++++++++++++++++++++++++++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index 42bc153..481ed59 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -4,6 +4,7 @@
 struct pkt_buff;
 
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
+struct pkt_buff *pktb_make(int family, void *data, size_t len, size_t extra, void *buf, size_t bufsize);
 void pktb_free(struct pkt_buff *pktb);
 
 uint8_t *pktb_data(struct pkt_buff *pktb);
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 37f6bc0..d85121c 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -29,6 +29,8 @@
  * @{
  */
 
+static struct pkt_buff *pktb_alloc_make_common(int family, void *data,
+	size_t len, size_t extra, struct pkt_buff *pktb, bool malloc_used);
 /**
  * pktb_alloc - allocate a new packet buffer
  * \param family Indicate what family. Currently supported families are
@@ -52,11 +54,17 @@ EXPORT_SYMBOL
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 {
 	struct pkt_buff *pktb;
-	void *pkt_data;
 
 	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
 	if (pktb == NULL)
 		return NULL;
+	return pktb_alloc_make_common(family, data, len, extra, pktb, true);
+}
+
+static struct pkt_buff *pktb_alloc_make_common(int family, void *data,
+	size_t len, size_t extra, struct pkt_buff *pktb, bool malloc_used)
+{
+	void *pkt_data;
 
 	/* Better make sure alignment is correct. */
 	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
@@ -87,7 +95,8 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 		default:
 			/* This protocol is unsupported. */
 			errno = EPROTONOSUPPORT;
-			free(pktb);
+			if (malloc_used)
+				free(pktb);
 			return NULL;
 		}
 		break;
@@ -96,6 +105,53 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	return pktb;
 }
 
+/**
+ * pktb_make - make a packet buffer from an existing buffer
+ * \param family Indicate what family. Currently supported families are
+ * AF_BRIDGE, AF_INET & AF_INET6.
+ * \param data Pointer to packet data
+ * \param len Packet length
+ * \param extra Extra memory in the tail to be allocated (for mangling)
+ * \param buf Existing buffer to use
+ * \param bufsize Size of _buf_
+ *
+ * This function builds a userspace packet buffer inside a supplied buffer.
+ * The packet buffer contains the packet data and some extra memory room in the
+ * tail (if requested).
+ *
+ * This function uses less CPU cycles than pktb_alloc() + pktb_free().
+ * Users can expect a decrease in overall CPU time in the order of 5%.
+ *
+ * \return Pointer to a userspace packet buffer aligned within _buf_ or NULL on
+ * failure.
+ * \par Errors
+ * __ENOMEM__ _bufsize_ is too small to accomodate _len_
+ * \n
+ * __EPROTONOSUPPORT__ _family_ was __AF_BRIDGE__ and this is not an IP packet
+ * (v4 or v6)
+ * \warning Do __not__ call pktb_free() on the returned pointer
+ */
+EXPORT_SYMBOL
+struct pkt_buff *pktb_make(int family, void *data, size_t len, size_t extra,
+			     void *buf, size_t bufsize)
+{
+	struct pkt_buff *pktb;
+
+	/* Better make sure alignment is correct. */
+	size_t extra2 =
+		(size_t)buf & 0x7 ? (~((size_t)buf & 0x7) & 0x7) + 1 : 0;
+
+	pktb = buf + extra2;
+	if (extra2 + sizeof(struct pkt_buff) + len + extra > bufsize)
+	{
+		errno = ENOMEM;
+		return NULL;
+	}
+	memset(pktb, 0, sizeof(struct pkt_buff) + len + extra);
+
+	return pktb_alloc_make_common(family, data, len, extra, pktb, false);
+}
+
 /**
  * pktb_data - get pointer to network packet
  * \param pktb Pointer to userspace packet buffer
-- 
2.14.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH libnetfilter_queue v2 0/1] New pktb_make() function
  2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 0/1] New pktb_make() function Duncan Roe
@ 2020-01-08 22:53   ` Pablo Neira Ayuso
  2020-01-10  2:27     ` Duncan Roe
  0 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-08 22:53 UTC (permalink / raw)
  To: Duncan Roe; +Cc: netfilter-devel

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On Mon, Jan 06, 2020 at 02:17:13PM +1100, Duncan Roe wrote:
> This patch offers a faster alternative / replacement function to pktb_alloc().
> 
> pktb_make() is a copy of the first part of pktb_alloc() modified to use a
> supplied buffer rather than calloc() one. It then calls the second part of
> pktb_alloc() which is modified to be a static function.
> 
> Can't think of a use case where one would choose to use pktb_alloc over
> pktb_make.
> In a furure documentation update, might relegate pktb_alloc and pktb_free to
> "other functions".

This is very useful.

Would you explore something looking similar to what I'm attaching?

Warning: Compile tested only :-)

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2901 bytes --]

diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 6250fbf3ac8b..985bb48ac986 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -29,6 +29,58 @@
  * @{
  */
 
+static struct pkt_buff *__pktb_alloc(int family, void *data, size_t len,
+				     size_t extra)
+{
+	struct pkt_buff *pktb;
+
+	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
+	if (pktb == NULL)
+		return NULL;
+
+	return pktb;
+}
+
+static int pktb_setup_family(struct pkt_buff *pktb, int family)
+{
+	switch(family) {
+	case AF_INET:
+	case AF_INET6:
+		pktb->network_header = pktb->data;
+		break;
+	case AF_BRIDGE: {
+		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
+
+		pktb->mac_header = pktb->data;
+
+		switch(ethhdr->h_proto) {
+		case ETH_P_IP:
+		case ETH_P_IPV6:
+			pktb->network_header = pktb->data + ETH_HLEN;
+			break;
+		default:
+			/* This protocol is unsupported. */
+			errno = EPROTONOSUPPORT;
+			return -1;
+		}
+		break;
+		}
+	}
+
+	return 0;
+}
+
+static void pktb_setup_metadata(struct pkt_buff *pktb, void *pkt_data,
+				size_t len, size_t extra)
+{
+	pktb->len = len;
+	pktb->data_len = len + extra;
+
+	pktb->head = pkt_data;
+	pktb->data = pkt_data;
+	pktb->tail = pktb->head + len;
+}
+
 /**
  * pktb_alloc - allocate a new packet buffer
  * \param family Indicate what family. Currently supported families are
@@ -54,45 +106,41 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	struct pkt_buff *pktb;
 	void *pkt_data;
 
-	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
-	if (pktb == NULL)
+	pktb = __pktb_alloc(family, data, len, extra);
+	if (!pktb)
 		return NULL;
 
 	/* Better make sure alignment is correct. */
 	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
 	memcpy(pkt_data, data, len);
 
-	pktb->len = len;
-	pktb->data_len = len + extra;
+	pktb_setup_metadata(pktb, pkt_data, len, extra);
 
-	pktb->head = pkt_data;
-	pktb->data = pkt_data;
-	pktb->tail = pktb->head + len;
+	if (pktb_setup_family(pktb, family) < 0) {
+		free(pktb);
+		return NULL;
+	}
 
-	switch(family) {
-	case AF_INET:
-	case AF_INET6:
-		pktb->network_header = pktb->data;
-		break;
-	case AF_BRIDGE: {
-		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
+	return pktb;
+}
 
-		pktb->mac_header = pktb->data;
+EXPORT_SYMBOL
+struct pkt_buff *pktb_alloc_data(int family, void *data, size_t len)
+{
+	struct pkt_buff *pktb;
 
-		switch(ethhdr->h_proto) {
-		case ETH_P_IP:
-		case ETH_P_IPV6:
-			pktb->network_header = pktb->data + ETH_HLEN;
-			break;
-		default:
-			/* This protocol is unsupported. */
-			errno = EPROTONOSUPPORT;
-			free(pktb);
-			return NULL;
-		}
-		break;
-	}
+	pktb = __pktb_alloc(family, data, 0, 0);
+	if (!pktb)
+		return NULL;
+
+	pktb->data = data;
+	pktb_setup_metadata(pktb, data, len, 0);
+
+	if (pktb_setup_family(pktb, family) < 0) {
+		free(pktb);
+		return NULL;
 	}
+
 	return pktb;
 }
 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH libnetfilter_queue v2 0/1] New pktb_make() function
  2020-01-08 22:53   ` Pablo Neira Ayuso
@ 2020-01-10  2:27     ` Duncan Roe
  2020-01-13 18:51       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Duncan Roe @ 2020-01-10  2:27 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Netfilter Development

On Wed, Jan 08, 2020 at 11:53:23PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 06, 2020 at 02:17:13PM +1100, Duncan Roe wrote:
> > This patch offers a faster alternative / replacement function to pktb_alloc().
> >
> > pktb_make() is a copy of the first part of pktb_alloc() modified to use a
> > supplied buffer rather than calloc() one. It then calls the second part of
> > pktb_alloc() which is modified to be a static function.
> >
> > Can't think of a use case where one would choose to use pktb_alloc over
> > pktb_make.
> > In a furure documentation update, might relegate pktb_alloc and pktb_free to
> > "other functions".
>
> This is very useful.
>
> Would you explore something looking similar to what I'm attaching?
>
> Warning: Compile tested only :-)
>
> Thanks.

> diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> index 6250fbf3ac8b..985bb48ac986 100644
> --- a/src/extra/pktbuff.c
> +++ b/src/extra/pktbuff.c
> @@ -29,6 +29,58 @@
>   * @{
>   */
>
> +static struct pkt_buff *__pktb_alloc(int family, void *data, size_t len,
> +				     size_t extra)
> +{
> +	struct pkt_buff *pktb;
> +
> +	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
> +	if (pktb == NULL)
> +		return NULL;
> +
> +	return pktb;
> +}
> +
> +static int pktb_setup_family(struct pkt_buff *pktb, int family)
> +{
> +	switch(family) {
> +	case AF_INET:
> +	case AF_INET6:
> +		pktb->network_header = pktb->data;
> +		break;
> +	case AF_BRIDGE: {
> +		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
> +
> +		pktb->mac_header = pktb->data;
> +
> +		switch(ethhdr->h_proto) {
> +		case ETH_P_IP:
> +		case ETH_P_IPV6:
> +			pktb->network_header = pktb->data + ETH_HLEN;
> +			break;
> +		default:
> +			/* This protocol is unsupported. */
> +			errno = EPROTONOSUPPORT;
> +			return -1;
> +		}
> +		break;
> +		}

GRR! I just wasted 20 minutes looking at these last 3 lines. Fix the indentation

> +	}
> +
> +	return 0;
> +}
> +
> +static void pktb_setup_metadata(struct pkt_buff *pktb, void *pkt_data,
> +				size_t len, size_t extra)
> +{
> +	pktb->len = len;

You know, we only need any 2 of len, data and tail. This has caused bugs in the
past, e.g. commit 8a4316f31. In the code, len seems to be the least used - will
see if I can't get rid of it.

> +	pktb->data_len = len + extra;
> +
> +	pktb->head = pkt_data;
> +	pktb->data = pkt_data;
> +	pktb->tail = pktb->head + len;
> +}
> +
>  /**
>   * pktb_alloc - allocate a new packet buffer
>   * \param family Indicate what family. Currently supported families are
> @@ -54,45 +106,41 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>  	struct pkt_buff *pktb;
>  	void *pkt_data;
>
> -	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
> -	if (pktb == NULL)
> +	pktb = __pktb_alloc(family, data, len, extra);
> +	if (!pktb)
>  		return NULL;
>
>  	/* Better make sure alignment is correct. */
>  	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
>  	memcpy(pkt_data, data, len);
>
> -	pktb->len = len;
> -	pktb->data_len = len + extra;
> +	pktb_setup_metadata(pktb, pkt_data, len, extra);
>
> -	pktb->head = pkt_data;
> -	pktb->data = pkt_data;
> -	pktb->tail = pktb->head + len;
> +	if (pktb_setup_family(pktb, family) < 0) {
> +		free(pktb);
> +		return NULL;
> +	}
>
> -	switch(family) {
> -	case AF_INET:
> -	case AF_INET6:
> -		pktb->network_header = pktb->data;
> -		break;
> -	case AF_BRIDGE: {
> -		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
> +	return pktb;
> +}
>
> -		pktb->mac_header = pktb->data;
> +EXPORT_SYMBOL
> +struct pkt_buff *pktb_alloc_data(int family, void *data, size_t len)
> +{
> +	struct pkt_buff *pktb;
>
> -		switch(ethhdr->h_proto) {
> -		case ETH_P_IP:
> -		case ETH_P_IPV6:
> -			pktb->network_header = pktb->data + ETH_HLEN;
> -			break;
> -		default:
> -			/* This protocol is unsupported. */
> -			errno = EPROTONOSUPPORT;
> -			free(pktb);
> -			return NULL;
> -		}
> -		break;
> -	}
> +	pktb = __pktb_alloc(family, data, 0, 0);
> +	if (!pktb)
> +		return NULL;
> +
> +	pktb->data = data;
> +	pktb_setup_metadata(pktb, data, len, 0);
> +
> +	if (pktb_setup_family(pktb, family) < 0) {
> +		free(pktb);
> +		return NULL;
>  	}
> +
>  	return pktb;
>  }
>

Ok, so this is another approach to reducing CPU time: avoid memcpy of data.

That's great if you're not mangling content.

But if you are mangling, beware. pktb now has pointers into the buffer you used
for receiving from Netlink so you must use a different buffer when sending.

I'll be updating my performance testing regime to get results more quickly, so
will be reporting back at some stage. It remains the case that malloc / free
were taking at least 5% of overall CPU with short packets (and btw most of that
is free(): as a rule of thumb I have found free cpu = 3 * malloc cpu)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH libnetfilter_queue v2 0/1] New pktb_make() function
  2020-01-10  2:27     ` Duncan Roe
@ 2020-01-13 18:51       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-13 18:51 UTC (permalink / raw)
  To: Netfilter Development

On Fri, Jan 10, 2020 at 01:27:57PM +1100, Duncan Roe wrote:
> On Wed, Jan 08, 2020 at 11:53:23PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 06, 2020 at 02:17:13PM +1100, Duncan Roe wrote:
> > > This patch offers a faster alternative / replacement function to pktb_alloc().
> > >
> > > pktb_make() is a copy of the first part of pktb_alloc() modified to use a
> > > supplied buffer rather than calloc() one. It then calls the second part of
> > > pktb_alloc() which is modified to be a static function.
> > >
> > > Can't think of a use case where one would choose to use pktb_alloc over
> > > pktb_make.
> > > In a furure documentation update, might relegate pktb_alloc and pktb_free to
> > > "other functions".
> >
> > This is very useful.
> >
> > Would you explore something looking similar to what I'm attaching?
> >
> > Warning: Compile tested only :-)
> >
> > Thanks.
> 
> > diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> > index 6250fbf3ac8b..985bb48ac986 100644
> > --- a/src/extra/pktbuff.c
> > +++ b/src/extra/pktbuff.c
> > @@ -29,6 +29,58 @@
> >   * @{
> >   */
> >
> > +static struct pkt_buff *__pktb_alloc(int family, void *data, size_t len,
> > +				     size_t extra)
> > +{
> > +	struct pkt_buff *pktb;
> > +
> > +	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
> > +	if (pktb == NULL)
> > +		return NULL;
> > +
> > +	return pktb;
> > +}
> > +
> > +static int pktb_setup_family(struct pkt_buff *pktb, int family)
> > +{
> > +	switch(family) {
> > +	case AF_INET:
> > +	case AF_INET6:
> > +		pktb->network_header = pktb->data;
> > +		break;
> > +	case AF_BRIDGE: {
> > +		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
> > +
> > +		pktb->mac_header = pktb->data;
> > +
> > +		switch(ethhdr->h_proto) {
> > +		case ETH_P_IP:
> > +		case ETH_P_IPV6:
> > +			pktb->network_header = pktb->data + ETH_HLEN;
> > +			break;
> > +		default:
> > +			/* This protocol is unsupported. */
> > +			errno = EPROTONOSUPPORT;
> > +			return -1;
> > +		}
> > +		break;
> > +		}
> 
> GRR! I just wasted 20 minutes looking at these last 3 lines. Fix the indentation
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void pktb_setup_metadata(struct pkt_buff *pktb, void *pkt_data,
> > +				size_t len, size_t extra)
> > +{
> > +	pktb->len = len;
> 
> You know, we only need any 2 of len, data and tail. This has caused bugs in the
> past, e.g. commit 8a4316f31. In the code, len seems to be the least used - will
> see if I can't get rid of it.
> 
> > +	pktb->data_len = len + extra;
> > +
> > +	pktb->head = pkt_data;
> > +	pktb->data = pkt_data;
> > +	pktb->tail = pktb->head + len;
> > +}
> > +
> >  /**
> >   * pktb_alloc - allocate a new packet buffer
> >   * \param family Indicate what family. Currently supported families are
> > @@ -54,45 +106,41 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
> >  	struct pkt_buff *pktb;
> >  	void *pkt_data;
> >
> > -	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
> > -	if (pktb == NULL)
> > +	pktb = __pktb_alloc(family, data, len, extra);
> > +	if (!pktb)
> >  		return NULL;
> >
> >  	/* Better make sure alignment is correct. */
> >  	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
> >  	memcpy(pkt_data, data, len);
> >
> > -	pktb->len = len;
> > -	pktb->data_len = len + extra;
> > +	pktb_setup_metadata(pktb, pkt_data, len, extra);
> >
> > -	pktb->head = pkt_data;
> > -	pktb->data = pkt_data;
> > -	pktb->tail = pktb->head + len;
> > +	if (pktb_setup_family(pktb, family) < 0) {
> > +		free(pktb);
> > +		return NULL;
> > +	}
> >
> > -	switch(family) {
> > -	case AF_INET:
> > -	case AF_INET6:
> > -		pktb->network_header = pktb->data;
> > -		break;
> > -	case AF_BRIDGE: {
> > -		struct ethhdr *ethhdr = (struct ethhdr *)pktb->data;
> > +	return pktb;
> > +}
> >
> > -		pktb->mac_header = pktb->data;
> > +EXPORT_SYMBOL
> > +struct pkt_buff *pktb_alloc_data(int family, void *data, size_t len)
> > +{
> > +	struct pkt_buff *pktb;
> >
> > -		switch(ethhdr->h_proto) {
> > -		case ETH_P_IP:
> > -		case ETH_P_IPV6:
> > -			pktb->network_header = pktb->data + ETH_HLEN;
> > -			break;
> > -		default:
> > -			/* This protocol is unsupported. */
> > -			errno = EPROTONOSUPPORT;
> > -			free(pktb);
> > -			return NULL;
> > -		}
> > -		break;
> > -	}
> > +	pktb = __pktb_alloc(family, data, 0, 0);
> > +	if (!pktb)
> > +		return NULL;
> > +
> > +	pktb->data = data;
> > +	pktb_setup_metadata(pktb, data, len, 0);
> > +
> > +	if (pktb_setup_family(pktb, family) < 0) {
> > +		free(pktb);
> > +		return NULL;
> >  	}
> > +
> >  	return pktb;
> >  }
> >
> 
> Ok, so this is another approach to reducing CPU time: avoid memcpy of data.
> 
> That's great if you're not mangling content.
> 
> But if you are mangling, beware. pktb now has pointers into the buffer you used
> for receiving from Netlink so you must use a different buffer when sending.

Probably we can store a flag in the pkt_buff structure to say "this
buffer is now ours" so mangling can just trigger a clone via malloc()
+ memcpy() only for that path.

Or you can just document this, although handling this case for the
library would be make it easier to user for users.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10 11:26 [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
2019-12-10 11:26 ` [PATCH libnetfilter_queue 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe
2019-12-22  2:09 ` [PATCH libnetfilter_queue 0/1] New pktb_usebuf() function Duncan Roe
2020-01-03  2:47 ` Duncan Roe
2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 0/1] New pktb_make() function Duncan Roe
2020-01-08 22:53   ` Pablo Neira Ayuso
2020-01-10  2:27     ` Duncan Roe
2020-01-13 18:51       ` Pablo Neira Ayuso
2020-01-06  3:17 ` [PATCH libnetfilter_queue v2 1/1] src: Add alternative function to pktb_alloc to avoid malloc / free overhead Duncan Roe

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git