All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/1] Reusing modifier socket for bulk ct loads
@ 2022-06-02 16:34 Mikhail Sennikovsky
  2022-06-02 16:34 ` [PATCH 1/1] conntrack: use same modifier socket for bulk ops Mikhail Sennikovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Mikhail Sennikovsky @ 2022-06-02 16:34 UTC (permalink / raw)
  To: netfilter-devel, pablo, mikhail.sennikovsky; +Cc: Mikhail Sennikovsky

Hi Pablo & all,

As discussed, here is the follow-up patch to use the same
mnl modifier socket for all operation in bulk ct loads
to increase performance of such loads.

Any feedback/suggestions are welcome.

Regards,
Mikhail

Mikhail Sennikovsky (1):
  conntrack: use same modifier socket for bulk ops

 src/conntrack.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-02 16:34 [PATCH 0/1] Reusing modifier socket for bulk ct loads Mikhail Sennikovsky
@ 2022-06-02 16:34 ` Mikhail Sennikovsky
  2022-06-08  6:18   ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Mikhail Sennikovsky @ 2022-06-02 16:34 UTC (permalink / raw)
  To: netfilter-devel, pablo, mikhail.sennikovsky; +Cc: Mikhail Sennikovsky

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@ionos.com>
---
 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)
 			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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-02 16:34 ` [PATCH 1/1] conntrack: use same modifier socket for bulk ops Mikhail Sennikovsky
@ 2022-06-08  6:18   ` Pablo Neira Ayuso
  2022-06-08 11:02     ` Mikhail Sennikovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2022-06-08  6:18 UTC (permalink / raw)
  To: Mikhail Sennikovsky; +Cc: netfilter-devel, mikhail.sennikovsky

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@ionos.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-08  6:18   ` Pablo Neira Ayuso
@ 2022-06-08 11:02     ` Mikhail Sennikovsky
  2022-06-08 11:08       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Mikhail Sennikovsky @ 2022-06-08 11:02 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, mikhail.sennikovsky

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.

Regards,
Mikhail

On Wed, 8 Jun 2022 at 08:18, Pablo Neira Ayuso <pablo@netfilter.org> 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@ionos.com>
> > ---
> >  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
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-08 11:02     ` Mikhail Sennikovsky
@ 2022-06-08 11:08       ` Pablo Neira Ayuso
  2022-06-08 11:19         ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2022-06-08 11:08 UTC (permalink / raw)
  To: Mikhail Sennikovsky; +Cc: netfilter-devel, mikhail.sennikovsky

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?

> Regards,
> Mikhail
> 
> On Wed, 8 Jun 2022 at 08:18, Pablo Neira Ayuso <pablo@netfilter.org> 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@ionos.com>
> > > ---
> > >  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
> > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-08 11:08       ` Pablo Neira Ayuso
@ 2022-06-08 11:19         ` Pablo Neira Ayuso
  2022-06-08 14:16           ` Mikhail Sennikovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2022-06-08 11:19 UTC (permalink / raw)
  To: Mikhail Sennikovsky; +Cc: netfilter-devel, mikhail.sennikovsky

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@netfilter.org> 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@ionos.com>
> > > > ---
> > > >  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
> > > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-08 11:19         ` Pablo Neira Ayuso
@ 2022-06-08 14:16           ` Mikhail Sennikovsky
  2022-06-09 10:20             ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Mikhail Sennikovsky @ 2022-06-08 14:16 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, mikhail.sennikovsky

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@netfilter.org> 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@netfilter.org> 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@ionos.com>
> > > > > ---
> > > > >  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
> > > > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-08 14:16           ` Mikhail Sennikovsky
@ 2022-06-09 10:20             ` Pablo Neira Ayuso
  2022-06-09 10:42               ` Mikhail Sennikovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2022-06-09 10:20 UTC (permalink / raw)
  To: Mikhail Sennikovsky; +Cc: netfilter-devel, mikhail.sennikovsky

Hi,

On Wed, Jun 08, 2022 at 04:16:34PM +0200, Mikhail Sennikovsky wrote:
> 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.

If not used now for this usecase, I'd prefer if you remove it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] conntrack: use same modifier socket for bulk ops
  2022-06-09 10:20             ` Pablo Neira Ayuso
@ 2022-06-09 10:42               ` Mikhail Sennikovsky
  0 siblings, 0 replies; 9+ messages in thread
From: Mikhail Sennikovsky @ 2022-06-09 10:42 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, mikhail.sennikovsky

Sure, let me remove it then and submit an updated patch here.

Thanks,
Mikhail

On Thu, 9 Jun 2022 at 12:20, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi,
>
> On Wed, Jun 08, 2022 at 04:16:34PM +0200, Mikhail Sennikovsky wrote:
> > 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.
>
> If not used now for this usecase, I'd prefer if you remove it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-06-09 10:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 16:34 [PATCH 0/1] Reusing modifier socket for bulk ct loads Mikhail Sennikovsky
2022-06-02 16:34 ` [PATCH 1/1] conntrack: use same modifier socket for bulk ops Mikhail Sennikovsky
2022-06-08  6:18   ` Pablo Neira Ayuso
2022-06-08 11:02     ` Mikhail Sennikovsky
2022-06-08 11:08       ` Pablo Neira Ayuso
2022-06-08 11:19         ` Pablo Neira Ayuso
2022-06-08 14:16           ` Mikhail Sennikovsky
2022-06-09 10:20             ` Pablo Neira Ayuso
2022-06-09 10:42               ` Mikhail Sennikovsky

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.