Re: [PATCH libnetfilter_queue v2] src: Add faster alternatives to pktb_alloc()

[Duncan Roe <duncan_roe@optusnet.com.au>]

Hi Pablo,

On Wed, Feb 19, 2020 at 07:04:10PM +0100, Pablo Neira Ayuso wrote:
> On Sat, Feb 01, 2020 at 05:21:27PM +1100, Duncan Roe wrote:
[...]
> >  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
> > +struct pkt_buff *pktb_alloc_data(int family, void *data, size_t len);
> > +struct pkt_buff *pktb_make(int family, void *data, size_t len, size_t extra, void *buf, size_t bufsize);
> > +struct pkt_buff *pktb_make_data(int family, void *data, size_t len, void *buf, size_t bufsize);
>
> Hm, when I delivered the patch to you, I forgot that you main point
> was that you wanted to skip the memory allocation.
>
> I wonder if all these new functions can be consolidated into one
> single function, something like:
>
>         struct pkt_buff *pktb_alloc2(int family, void *head, size_t head_size, void *data, size_t len, size_t extra);
>
> The idea is that:
>
> * head is the memory area that is large enough for the struct pkt_buff
>   (metadata). You can add a new pktb_head_size() function that returns
>   the size of opaque struct pkt_buff structure (whose layout is not
>   exposed to the user). I think you can this void *buf in your pktb_make
>   function.
>
> * data is the memory area to store the network packet itself.
>
> Then, you can allocate head and data in the stack and skip
> malloc/calloc.
>
> Would this work for you? I would prefer if there is just one single
> advanced function to do this.
>
> Thanks for your patience.

This patch set is not the last word. It would really help my development process
if you could just apply the patch set as-is.

> I would prefer if there is just one single advanced function ...

There *is* only 1 "advanced" function listed on the "User-space network packet
buffer" web page: pktb_make_data.

pktb_alloc must be kept for legacy support but it's documented on the "Other
functions" page.

pktb_alloc_data was only written for the benefit of timing tests. I can send a
patch to withdraw it as soon as this set is accepted.

Or, I can submit v4 with pktb_alloc_data removed. But only if you agree you will
then commit the patch.

> * data is the memory area to store the network packet itself.

Eh?? The plan is to leave the data where it is. pktb_make_data does that, it's
fine to use unless mangling could increase the packet size.

A more advance set of functions could lift that restriction. I'm still keen to
investigate the savings to be had by not having to move packet data for an
advanced variant of nfq_nlmsg_verdict_put_pkt. The idea is to use a new
structure (essentially metadata plus a struct nlmsghdr) which I have tentatively
named struct nlmsg_buffer. A number of advanced funtion variants would use it in
place of struct nlmsghdr. Please LMK if you would be interested in this.

> Thanks for your patience.

I have been working on something else :/

I had to put libnetfilter_queue development on hold because juggling 3 branches
was just getting to be too much.

(others are https://www.spinics.net/lists/netfilter-devel/msg65661.html (man
pages) and https://www.spinics.net/lists/netfilter-devel/msg65585.html (add more
helper functions to simplify coding)).

I can send a man pages update as soon as you commit something.

Cheers ... Duncan.