| Message ID | 20220109031653.23835-6-duncan_roe@optusnet.com.au |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Speed-up | expand |
Guys, Withot this patchset it remains an embarassing fact that any nfq program that uses the deprecated libnfnetlink interface and examines packet data will use more CPU if rewritten to use the libmnl interface. Please apply the set or give feedback on it as a matter of urgency. Cheers ... Duncan.
Hi Duncan, On Tue, Jan 18, 2022 at 10:58:14AM +1100, Duncan Roe wrote: > Guys, > > Withot this patchset it remains an embarassing fact that any nfq program that > uses the deprecated libnfnetlink interface and examines packet data will use > more CPU if rewritten to use the libmnl interface. We should untagged as deprecated libnetfilter_queue old interface in the documentation, it's fine to keep it there, it should be possible to make it work over libmnl. > Please apply the set or give feedback on it as a matter of urgency. There is absolutely no urgency. This patch have a number of showstoppers such as exposing structure layout on the header files. I think the goal is to avoid the memcpy() I posted a patch sketch time ago to follow a more simple approach that we can resurrect, polish and upstream to remove the extra copy. Thanks.
On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > This patch have a number of showstoppers such as exposing structure > layout on the header files. > That's only in patch 5. You could apply 1-4. There are actually no other showstoppers, right? Cheers ... Duncan.
Duncan Roe <duncan_roe@optusnet.com.au> wrote: > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > This patch have a number of showstoppers such as exposing structure > > layout on the header files. > > > That's only in patch 5. You could apply 1-4. There are actually no other > showstoppers, right? Why does patch 3 exist? Shouldn't that just get squashed into patch 1?
Duncan Roe <duncan_roe@optusnet.com.au> wrote: > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > This patch have a number of showstoppers such as exposing structure > > layout on the header files. > > > That's only in patch 5. You could apply 1-4. There are actually no other > showstoppers, right? Regarding patch 5, I think its ok except the pkt_buff layout freeze. From a quick glance, there is no assumption that the data area resides after the pktbuff head, so it should be possible to keep pkt_buff private, allocate an empty packet and then associate a new buffer with it. I agree the memcpy needs to go, nfqueue uses should use F_GSO feature flag and memcpy'ing 60k big packets isn't ideal.
On Thu, Jan 20, 2022 at 01:04:58PM +0100, Florian Westphal wrote: > Duncan Roe <duncan_roe@optusnet.com.au> wrote: > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > > > This patch have a number of showstoppers such as exposing structure > > > layout on the header files. > > > > > That's only in patch 5. You could apply 1-4. There are actually no other > > showstoppers, right? > > Regarding patch 5, I think its ok except the pkt_buff layout freeze. > > From a quick glance, there is no assumption that the data area resides > after the pktbuff head, so it should be possible to keep pkt_buff > private, allocate an empty packet and then associate a new buffer with > it. Or allocate pktbuff offline and recycle it (re-setup) on new packets coming from the kernel, it does not need to be allocated in the stack and exposing the layout is also therefore not requireed.
On Thu, Jan 20, 2022 at 07:27:25AM +0100, Florian Westphal wrote: > Duncan Roe <duncan_roe@optusnet.com.au> wrote: > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > > > This patch have a number of showstoppers such as exposing structure > > > layout on the header files. > > > > > That's only in patch 5. You could apply 1-4. There are actually no other > > showstoppers, right? > > Why does patch 3 exist? Shouldn't that just get squashed into patch 1? Didn't think of that. I have a squashed version now. Cheers ... Duncan.
On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > We should untagged as deprecated libnetfilter_queue old interface in > the documentation, it's fine to keep it there, it should be possible > to make it work over libmnl. > Agreed. The question is, what to do instead. We need *some* form of tagging to show whih are old interface and which are new interface functions. You write a program using either old or new: they don't mix. It's an early design decision which to use so we need to make it as clear as we can. Perhaps tag every module as NEW or OLD, with an explanation near the top of the Main page as to what is the difference. Ideally would have libnetfilter_queue.7 man page as well. Suggestions welcome, Cheers ... Duncan.
On Fri, Jan 21, 2022 at 01:36:34PM +1100, Duncan Roe wrote: > On Thu, Jan 20, 2022 at 07:27:25AM +0100, Florian Westphal wrote: > > Duncan Roe <duncan_roe@optusnet.com.au> wrote: > > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > > > > > This patch have a number of showstoppers such as exposing structure > > > > layout on the header files. > > > > > > > That's only in patch 5. You could apply 1-4. There are actually no other > > > showstoppers, right? > > > > Why does patch 3 exist? Shouldn't that just get squashed into patch 1? > > Didn't think of that. I have a squashed version now. > Also squashed patch 2 into patch 1 Cheers ... Duncan.
On Thu, Jan 20, 2022 at 01:04:58PM +0100, Florian Westphal wrote: > Duncan Roe <duncan_roe@optusnet.com.au> wrote: > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > > > This patch have a number of showstoppers such as exposing structure > > > layout on the header files. > > > > > That's only in patch 5. You could apply 1-4. There are actually no other > > showstoppers, right? > > Regarding patch 5, I think its ok except the pkt_buff layout freeze. > > From a quick glance, there is no assumption that the data area resides > after the pktbuff head, so it should be possible to keep pkt_buff > private, allocate an empty packet and then associate a new buffer with > it. > > I agree the memcpy needs to go, nfqueue uses should use F_GSO feature > flag and memcpy'ing 60k big packets isn't ideal. There is no pkt_buff layout freeze. If we want to change it in future, we bump the major version of libnetfilter_queue.so, same as we would do if changing the signature of an existing function. Or am I missing something? Cheers ... Duncan.
Duncan Roe <duncan_roe@optusnet.com.au> wrote: > There is no pkt_buff layout freeze. If we want to change it in future, we bump > the major version of libnetfilter_queue.so, same as we would do if changing the > signature of an existing function. ABI breakage is bad, just say no.
On Thu, Jan 20, 2022 at 01:40:09PM +0100, Pablo Neira Ayuso wrote: > On Thu, Jan 20, 2022 at 01:04:58PM +0100, Florian Westphal wrote: > > Duncan Roe <duncan_roe@optusnet.com.au> wrote: > > > On Tue, Jan 18, 2022 at 02:11:43AM +0100, Pablo Neira Ayuso wrote: > > > > > > > > This patch have a number of showstoppers such as exposing structure > > > > layout on the header files. > > > > > > > That's only in patch 5. You could apply 1-4. There are actually no other > > > showstoppers, right? > > > > Regarding patch 5, I think its ok except the pkt_buff layout freeze. > > > > From a quick glance, there is no assumption that the data area resides > > after the pktbuff head, so it should be possible to keep pkt_buff > > private, allocate an empty packet and then associate a new buffer with > > it. > > Or allocate pktbuff offline and recycle it (re-setup) on new packets > coming from the kernel, it does not need to be allocated in the > stack and exposing the layout is also therefore not requireed. I put it on the stack to get thread locality w/out the (admittedly small) overhead of using thread_local. We could have a pktbuff_size() to dimension it and keep the struct opaque. It's 48 bytes (64 bit) so I thought it was OK on the stack but you could malloc() it and only have a pointer (again on the stack for thread locality). Cheers ... Duncan.
diff --git a/examples/nf-queue.c b/examples/nf-queue.c index 4074e5a..f330b97 100644 --- a/examples/nf-queue.c +++ b/examples/nf-queue.c @@ -49,11 +49,12 @@ nfq_send_verdict(int queue_num, uint32_t id) } static int queue_cb(const struct nlmsghdr *nlh, void *data, - struct pkt_buff *supplied_pktb, size_t supplied_extra) + size_t supplied_extra) { struct nfqnl_msg_packet_hdr *ph = NULL; struct nlattr *attr[NFQA_MAX+1] = {}; struct pkt_buff *pktb; + struct pkt_buff pkt_b; uint32_t id = 0, skbinfo; struct nfgenmsg *nfg; uint8_t *payload; @@ -77,8 +78,7 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data, plen = mnl_attr_get_payload_len(attr[NFQA_PAYLOAD]); payload = mnl_attr_get_payload(attr[NFQA_PAYLOAD]); - pktb = pktb_populate(supplied_pktb, AF_INET, payload, plen, - supplied_extra); + pktb = pktb_populate(&pkt_b, AF_INET, payload, plen, supplied_extra); skbinfo = attr[NFQA_SKB_INFO] ? ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO])) : 0; diff --git a/include/libnetfilter_queue/callback.h b/include/libnetfilter_queue/callback.h index 27bfe31..756cb38 100644 --- a/include/libnetfilter_queue/callback.h +++ b/include/libnetfilter_queue/callback.h @@ -4,7 +4,7 @@ struct nlattr; struct pkt_buff; -typedef int (*nfq_cb_t)(const struct nlmsghdr *nlh, void *data, struct pkt_buff *pktb, size_t extra); +typedef int (*nfq_cb_t)(const struct nlmsghdr *nlh, void *data, size_t extra); int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap, unsigned int portid, nfq_cb_t cb_data, void *data); diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h index 33829cc..7ad7cb1 100644 --- a/include/libnetfilter_queue/pktbuff.h +++ b/include/libnetfilter_queue/pktbuff.h @@ -1,7 +1,18 @@ #ifndef _PKTBUFF_H_ #define _PKTBUFF_H_ -struct pkt_buff; +struct pkt_buff { + uint8_t *mac_header; + uint8_t *network_header; + uint8_t *transport_header; + + uint8_t *data; + + uint32_t len; + uint32_t data_len; + + bool mangled; +}; struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra); void pktb_free(struct pkt_buff *pktb); diff --git a/src/extra/callback.c b/src/extra/callback.c index dee6fc2..23e88f7 100644 --- a/src/extra/callback.c +++ b/src/extra/callback.c @@ -31,10 +31,9 @@ struct data_carrier static int local_cb(const struct nlmsghdr *nlh, void *data) { - struct pkt_buff pktb_instance = { }; struct data_carrier *d = (struct data_carrier *)data; - return d->cb_func(nlh, d->data, &pktb_instance, d->bufcap - d->buflen); + return d->cb_func(nlh, d->data, d->bufcap - d->buflen); } EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap, @@ -54,6 +53,5 @@ EXPORT_SYMBOL int nfq_cb_run(const void *buf, size_t buflen, size_t bufcap, return MNL_CB_ERROR; } - return mnl_cb_run(buf, buflen, 0, portid, local_cb, &dc); } diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c index 86d8fe6..ae3e454 100644 --- a/src/extra/pktbuff.c +++ b/src/extra/pktbuff.c @@ -125,6 +125,7 @@ EXPORT_SYMBOL struct pkt_buff *pktb_populate(struct pkt_buff *pktb, int family, void *data, size_t len, size_t extra) { + memset(pktb, 0, sizeof *pktb); pktb_setup_metadata(pktb, data, len, extra); if (__pktb_setup(family, pktb) < 0) pktb = NULL; diff --git a/src/internal.h b/src/internal.h index ae849d6..6adf3d1 100644 --- a/src/internal.h +++ b/src/internal.h @@ -4,6 +4,7 @@ #include "config.h" #include <stdint.h> #include <stdbool.h> +#include <libnetfilter_queue/pktbuff.h> #ifdef HAVE_VISIBILITY_HIDDEN # define EXPORT_SYMBOL __attribute__((visibility("default"))) #else @@ -18,19 +19,6 @@ uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph, uint16_t protonum); uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr, uint16_t protonum); -struct pkt_buff { - uint8_t *mac_header; - uint8_t *network_header; - uint8_t *transport_header; - - uint8_t *data; - - uint32_t len; - uint32_t data_len; - - bool mangled; -}; - static inline uint8_t *pktb_tail(struct pkt_buff *pktb) { return pktb->data + pktb->len;
With the advent of nfq_run_cb() and pktb_populate(), there is no longer ever a buffer tacked on the end of a struct pktbuff. Now that struct pktbuff is purely a buffer descriptor, there seems little point in keeping it opaque so expose it. Some code simplification ensues: the new callback function prototype only differs from the old one in having 1 extra arg being available free space. An application creates a struct pktbuff instance by just declaring it. pktb_populate() zeroises pktb because it's no longer guaranteed zero. As before, no new doxygen documentation until function prototypes are agreed, but examples/nf-queue.c is updated. Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au> --- v3: New patch examples/nf-queue.c | 6 +++--- include/libnetfilter_queue/callback.h | 2 +- include/libnetfilter_queue/pktbuff.h | 13 ++++++++++++- src/extra/callback.c | 4 +--- src/extra/pktbuff.c | 1 + src/internal.h | 14 +------------- 6 files changed, 19 insertions(+), 21 deletions(-)