Hi Pablo,
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;
> + }
> + }
> +
> + 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;
> }
>
The results are in. The spreadsheet is available at
https://github.com/duncan-roe/nfq/blob/master/nfq6/nfq6_timings.ods, but I'll
summarise them here.
Four functions are compared: pktb_alloc(), pktb_alloc_data(), pktb_make() and
pktb_make_data(). The timings for pktb_alloc & pktb_alloc_data include a call to
pktb_free(). When using pktb_make, one must not call pktb_free.
The test is to time calling the function (pair) 100000000 (1e8) times. In all
cases, this takes < 60 seconds. Two series of tests: one with 50-byte packets
(udp/IPv6, 2 data chars) and the other with 1500-byte (max MTU) packets.
Part-way through testing, pktb_make was improved to not zeroise the area into
which packet data will be copied. This is actually a tiny performance hit at
50-byte, could determine a crossover point at which to zeroise in 1 go but
didn't consider the extra complexity to be warrantied.
Here's a table of percentages:
bytes|pktb_alloc|pktb_alloc_data|pktb_make|pktb_make_data
-----|----------|---------------|---------|--------------
50| 100| 56.2| 50.8| 28.9
-----|----------|---------------|---------|--------------
1500| 100| 22.9| 59.6| 11.5
I have yet to do the doxygen documentation for the _data variants, then can send
in the code.
Cheers ... Duncan.
