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.
