Hi Pablo,
Then I misunderstood you, my bad.
Yes, _check is never used for events, and the socket->events is not
used anywhere except the assert(events == socket->events); assertion
check which I found useful as a sanity check for potential future uses
of the nfct_mnl_socket_check_open.
If you find it unneeded however, I'm fine with removing it.
Mikhail
On Wed, 8 Jun 2022 at 13:19, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 08, 2022 at 01:08:56PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Jun 08, 2022 at 01:02:30PM +0200, Mikhail Sennikovsky wrote:
> > > Hi Pablo,
> > >
> > > IIRC we never had a code that was using nfct_mnl_socket_check_open for
> > > CT_EVENT and friends, because it is not needed there, as they can not
> > > be used in the bulk operations.
> >
> > Not sure I follow. My question is: is this new events field really required?
>
> sock->mnl is always set on at the beginning of the batch processing.
> That remains unaltered.
>
> Then we have the modifier socket, which is used for update and delete.
> This is conditionally opened. The new _check function just checks if
> this socket is open, if so re-use it.
>
> _check is now never used for events.
>
> Correct?
>
> > > Regards,
> > > Mikhail
> > >
> > > On Wed, 8 Jun 2022 at 08:18, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > >
> > > > Hi Mikhail,
> > > >
> > > > On Thu, Jun 02, 2022 at 06:34:29PM +0200, Mikhail Sennikovsky wrote:
> > > > > For bulk ct entry loads (with -R option) reusing the same mnl
> > > > > modifier socket for all entries results in reduction of entries
> > > > > creation time, which becomes especially signifficant when loading
> > > > > tens of thouthand of entries.
> > > > >
> > > > > Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@xxxxxxxxx>
> > > > > ---
> > > > > src/conntrack.c | 31 +++++++++++++++++++++++++------
> > > > > 1 file changed, 25 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/src/conntrack.c b/src/conntrack.c
> > > > > index 27e2bea..be8690b 100644
> > > > > --- a/src/conntrack.c
> > > > > +++ b/src/conntrack.c
> > > > > @@ -71,6 +71,7 @@
> > > > > struct nfct_mnl_socket {
> > > > > struct mnl_socket *mnl;
> > > > > uint32_t portid;
> > > > > + uint32_t events;
> > > > > };
> > > > >
> > > > > static struct nfct_mnl_socket _sock;
> > > > > @@ -2441,6 +2442,7 @@ static int nfct_mnl_socket_open(struct nfct_mnl_socket *socket,
> > > > > return -1;
> > > > > }
> > > > > socket->portid = mnl_socket_get_portid(socket->mnl);
> > > > > + socket->events = events;
> > > > >
> > > > > return 0;
> > > > > }
> > > > > @@ -2470,6 +2472,25 @@ static void nfct_mnl_socket_close(const struct nfct_mnl_socket *sock)
> > > > > mnl_socket_close(sock->mnl);
> > > > > }
> > > > >
> > > > > +static int nfct_mnl_socket_check_open(struct nfct_mnl_socket *socket,
> > > > > + unsigned int events)
> > > > > +{
> > > > > + if (socket->mnl != NULL) {
> > > > > + assert(events == socket->events);
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + return nfct_mnl_socket_open(socket, events);
> > > > > +}
> > > > > +
> > > > > +static void nfct_mnl_socket_check_close(struct nfct_mnl_socket *sock)
> > > > > +{
> > > > > + if (sock->mnl) {
> > > > > + nfct_mnl_socket_close(sock);
> > > > > + memset(sock, 0, sizeof(*sock));
> > > > > + }
> > > > > +}
> > > > > +
> > > > > static int __nfct_mnl_dump(struct nfct_mnl_socket *sock,
> > > > > const struct nlmsghdr *nlh, mnl_cb_t cb, void *data)
> > > > > {
> > > > > @@ -3383,19 +3404,17 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd,
> > > > > break;
> > > > >
> > > > > case CT_UPDATE:
> > > > > - if (nfct_mnl_socket_open(modifier_sock, 0) < 0)
> > > > > + if (nfct_mnl_socket_check_open(modifier_sock, 0) < 0)
> > > > > exit_error(OTHER_PROBLEM, "Can't open handler");
> > > > >
> > > > > nfct_filter_init(cmd);
> > > > > res = nfct_mnl_dump(sock, NFNL_SUBSYS_CTNETLINK,
>
>
>
>
>
>
> > > > > IPCTNL_MSG_CT_GET, mnl_nfct_update_cb,
> > > > > cmd, NULL);
> > > > > -
> > > > > - nfct_mnl_socket_close(modifier_sock);
> > > > > break;
> > > > >
> > > > > case CT_DELETE:
> > > > > - if (nfct_mnl_socket_open(modifier_sock, 0) < 0)
> > > > > + if (nfct_mnl_socket_check_open(modifier_sock, 0) < 0)
> > > >
> > > > No events needed anymore?
> > > >
> > > > nfct_mnl_socket_check_open() is now only used by CT_UPDATE and CT_DELETE,
> > > > right?
> > > >
> > > > > exit_error(OTHER_PROBLEM, "Can't open handler");
> > > > >
> > > > > nfct_filter_init(cmd);
> > > > > @@ -3418,8 +3437,6 @@ static int do_command_ct(const char *progname, struct ct_cmd *cmd,
> > > > > cmd, filter_dump);
> > > > >
> > > > > nfct_filter_dump_destroy(filter_dump);
> > > > > -
> > > > > - nfct_mnl_socket_close(modifier_sock);
> > > > > break;
> > > > >
> > > > > case EXP_DELETE:
> > > > > @@ -3857,6 +3874,7 @@ static const char *ct_unsupp_cmd_file(const struct ct_cmd *cmd)
> > > > > int main(int argc, char *argv[])
> > > > > {
> > > > > struct nfct_mnl_socket *sock = &_sock;
> > > > > + struct nfct_mnl_socket *modifier_sock = &_modifier_sock;
> > > > > struct ct_cmd *cmd, *next;
> > > > > LIST_HEAD(cmd_list);
> > > > > int res = 0;
> > > > > @@ -3900,6 +3918,7 @@ int main(int argc, char *argv[])
> > > > > free(cmd);
> > > > > }
> > > > > nfct_mnl_socket_close(sock);
> > > > > + nfct_mnl_socket_check_close(modifier_sock);
> > > > >
> > > > > return res < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
> > > > > }
> > > > > --
> > > > > 2.25.1
> > > > >
