[3/3] net/mlx5: fix rule cleanup Netlink command sending
Checks
Commit Message
The VXLAN related rule cleanup routine queries and gathers all
existing local IP and neigh rules into buffer list. One buffer
may contain multiple rule deletetion commands and is prepared
to send into Netlink as single message. But, if error occurs
for some deletion commands in the buffer, the multiple ACK
message with errors can be send back by the kernel. It breaks
the Netlink communication sequence numbers, because we expect
only one ACK message and it smashes out futher Netlik
communication.
The workaround of this problem is to send rule deletion commands
from buffer in one-by-one fashion and get ACK message for every
command sent. We do not expect too may rules preexist, so there
should not be critical performance degradation at VXLAN outer
interface initialization.
Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup routines")
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
drivers/net/mlx5/mlx5_flow_tcf.c | 58 +++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 34 deletions(-)
Comments
> On Nov 10, 2018, at 1:59 AM, Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
>
> The VXLAN related rule cleanup routine queries and gathers all
> existing local IP and neigh rules into buffer list. One buffer
> may contain multiple rule deletetion commands and is prepared
> to send into Netlink as single message. But, if error occurs
> for some deletion commands in the buffer, the multiple ACK
> message with errors can be send back by the kernel. It breaks
> the Netlink communication sequence numbers, because we expect
> only one ACK message and it smashes out futher Netlik
> communication.
Just curious.
Is parsing the multiple ack msgs more complex than sending commands
one by one?
> The workaround of this problem is to send rule deletion commands
> from buffer in one-by-one fashion and get ACK message for every
> command sent. We do not expect too may rules preexist, so there
> should not be critical performance degradation at VXLAN outer
> interface initialization.
>
> Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup routines")
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>
Thanks
> drivers/net/mlx5/mlx5_flow_tcf.c | 58 +++++++++++++++++-----------------------
> 1 file changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
> index bba8aed..21eb99e 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -3847,30 +3847,6 @@ struct tcf_nlcb_context {
> }
>
> /**
> - * Set NLM_F_ACK flags in the last netlink command in buffer.
> - * Only last command in the buffer will be acked by system.
> - *
> - * @param[in, out] buf
> - * Pointer to buffer with netlink commands.
> - */
> -static void
> -flow_tcf_setack_nlcmd(struct tcf_nlcb_buf *buf)
> -{
> - struct nlmsghdr *nlh;
> - uint32_t size = 0;
> -
> - assert(buf->size);
> - do {
> - nlh = (struct nlmsghdr *)&buf->msg[size];
> - size += NLMSG_ALIGN(nlh->nlmsg_len);
> - if (size >= buf->size) {
> - nlh->nlmsg_flags |= NLM_F_ACK;
> - break;
> - }
> - } while (true);
> -}
> -
> -/**
> * Send the buffers with prepared netlink commands. Scans the list and
> * sends all found buffers. Buffers are sent and freed anyway in order
> * to prevent memory leakage if some every message in received packet.
> @@ -3888,21 +3864,35 @@ struct tcf_nlcb_context {
> flow_tcf_send_nlcmd(struct mlx5_flow_tcf_context *tcf,
> struct tcf_nlcb_context *ctx)
> {
> - struct tcf_nlcb_buf *bc, *bn;
> - struct nlmsghdr *nlh;
> + struct tcf_nlcb_buf *bc = LIST_FIRST(&ctx->nlbuf);
> int ret = 0;
>
> - bc = LIST_FIRST(&ctx->nlbuf);
> while (bc) {
> + struct tcf_nlcb_buf *bn = LIST_NEXT(bc, next);
> + struct nlmsghdr *nlh;
> + uint32_t msg = 0;
> int rc;
>
> - bn = LIST_NEXT(bc, next);
> - if (bc->size) {
> - flow_tcf_setack_nlcmd(bc);
> - nlh = (struct nlmsghdr *)&bc->msg;
> - rc = flow_tcf_nl_ack(tcf, nlh, bc->size, NULL, NULL);
> - if (rc && !ret)
> - ret = rc;
> + while (msg < bc->size) {
> + /*
> + * Send Netlink commands from buffer in one by one
> + * fashion. If we send multiple rule deletion commands
> + * in one Netlink message and some error occurs it may
> + * cause multiple ACK error messages and break sequence
> + * numbers of Netlink communication, because we expect
> + * the only one ACK reply.
> + */
> + assert((bc->size - msg) >= sizeof(struct nlmsghdr));
> + nlh = (struct nlmsghdr *)&bc->msg[msg];
> + assert((bc->size - msg) >= nlh->nlmsg_len);
> + msg += nlh->nlmsg_len;
> + rc = flow_tcf_nl_ack(tcf, nlh, 0, NULL, NULL);
> + if (rc) {
> + DRV_LOG(WARNING,
> + "netlink: cleanup error %d", rc);
> + if (!ret)
> + ret = rc;
> + }
> }
> rte_free(bc);
> bc = bn;
> --
> 1.8.3.1
>
> -----Original Message-----
> From: Yongseok Koh
> Sent: Sunday, November 11, 2018 13:42
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH 3/3] net/mlx5: fix rule cleanup Netlink command sending
>
>
> > On Nov 10, 2018, at 1:59 AM, Slava Ovsiienko
> <viacheslavo@mellanox.com> wrote:
> >
> > The VXLAN related rule cleanup routine queries and gathers all
> > existing local IP and neigh rules into buffer list. One buffer may
> > contain multiple rule deletetion commands and is prepared to send into
> > Netlink as single message. But, if error occurs for some deletion
> > commands in the buffer, the multiple ACK message with errors can be
> > send back by the kernel. It breaks the Netlink communication sequence
> > numbers, because we expect only one ACK message and it smashes out
> > futher Netlik communication.
>
> Just curious.
> Is parsing the multiple ack msgs more complex than sending commands one
> by one?
We are in the midst of send query/get dump process. We can't send
another request and wait ack for it - we are receiving the dump. Possible,
it can be done via creation one more Netlink socket, but I'm not sure the
requests are not queued by kernel. So - the simplest way - gather dump
and then send commands.
PS. Actually I have refactored gathering/sending, we need to gather
parameters only, not build entire commands in callbacks, but this patch
is not tested yet and too large as for simple fix.
WBR,
Slava
>
> > The workaround of this problem is to send rule deletion commands from
> > buffer in one-by-one fashion and get ACK message for every command
> > sent. We do not expect too may rules preexist, so there should not be
> > critical performance degradation at VXLAN outer interface
> > initialization.
> >
> > Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup
> > routines")
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
>
> Thanks
>
> > drivers/net/mlx5/mlx5_flow_tcf.c | 58
> > +++++++++++++++++-----------------------
> > 1 file changed, 24 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c
> > b/drivers/net/mlx5/mlx5_flow_tcf.c
> > index bba8aed..21eb99e 100644
> > --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> > +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> > @@ -3847,30 +3847,6 @@ struct tcf_nlcb_context { }
> >
> > /**
> > - * Set NLM_F_ACK flags in the last netlink command in buffer.
> > - * Only last command in the buffer will be acked by system.
> > - *
> > - * @param[in, out] buf
> > - * Pointer to buffer with netlink commands.
> > - */
> > -static void
> > -flow_tcf_setack_nlcmd(struct tcf_nlcb_buf *buf) -{
> > - struct nlmsghdr *nlh;
> > - uint32_t size = 0;
> > -
> > - assert(buf->size);
> > - do {
> > - nlh = (struct nlmsghdr *)&buf->msg[size];
> > - size += NLMSG_ALIGN(nlh->nlmsg_len);
> > - if (size >= buf->size) {
> > - nlh->nlmsg_flags |= NLM_F_ACK;
> > - break;
> > - }
> > - } while (true);
> > -}
> > -
> > -/**
> > * Send the buffers with prepared netlink commands. Scans the list and
> > * sends all found buffers. Buffers are sent and freed anyway in order
> > * to prevent memory leakage if some every message in received packet.
> > @@ -3888,21 +3864,35 @@ struct tcf_nlcb_context {
> > flow_tcf_send_nlcmd(struct mlx5_flow_tcf_context *tcf,
> > struct tcf_nlcb_context *ctx)
> > {
> > - struct tcf_nlcb_buf *bc, *bn;
> > - struct nlmsghdr *nlh;
> > + struct tcf_nlcb_buf *bc = LIST_FIRST(&ctx->nlbuf);
> > int ret = 0;
> >
> > - bc = LIST_FIRST(&ctx->nlbuf);
> > while (bc) {
> > + struct tcf_nlcb_buf *bn = LIST_NEXT(bc, next);
> > + struct nlmsghdr *nlh;
> > + uint32_t msg = 0;
> > int rc;
> >
> > - bn = LIST_NEXT(bc, next);
> > - if (bc->size) {
> > - flow_tcf_setack_nlcmd(bc);
> > - nlh = (struct nlmsghdr *)&bc->msg;
> > - rc = flow_tcf_nl_ack(tcf, nlh, bc->size, NULL, NULL);
> > - if (rc && !ret)
> > - ret = rc;
> > + while (msg < bc->size) {
> > + /*
> > + * Send Netlink commands from buffer in one by one
> > + * fashion. If we send multiple rule deletion
> commands
> > + * in one Netlink message and some error occurs it
> may
> > + * cause multiple ACK error messages and break
> sequence
> > + * numbers of Netlink communication, because we
> expect
> > + * the only one ACK reply.
> > + */
> > + assert((bc->size - msg) >= sizeof(struct nlmsghdr));
> > + nlh = (struct nlmsghdr *)&bc->msg[msg];
> > + assert((bc->size - msg) >= nlh->nlmsg_len);
> > + msg += nlh->nlmsg_len;
> > + rc = flow_tcf_nl_ack(tcf, nlh, 0, NULL, NULL);
> > + if (rc) {
> > + DRV_LOG(WARNING,
> > + "netlink: cleanup error %d", rc);
> > + if (!ret)
> > + ret = rc;
> > + }
> > }
> > rte_free(bc);
> > bc = bn;
> > --
> > 1.8.3.1
> >
@@ -3847,30 +3847,6 @@ struct tcf_nlcb_context {
}
/**
- * Set NLM_F_ACK flags in the last netlink command in buffer.
- * Only last command in the buffer will be acked by system.
- *
- * @param[in, out] buf
- * Pointer to buffer with netlink commands.
- */
-static void
-flow_tcf_setack_nlcmd(struct tcf_nlcb_buf *buf)
-{
- struct nlmsghdr *nlh;
- uint32_t size = 0;
-
- assert(buf->size);
- do {
- nlh = (struct nlmsghdr *)&buf->msg[size];
- size += NLMSG_ALIGN(nlh->nlmsg_len);
- if (size >= buf->size) {
- nlh->nlmsg_flags |= NLM_F_ACK;
- break;
- }
- } while (true);
-}
-
-/**
* Send the buffers with prepared netlink commands. Scans the list and
* sends all found buffers. Buffers are sent and freed anyway in order
* to prevent memory leakage if some every message in received packet.
@@ -3888,21 +3864,35 @@ struct tcf_nlcb_context {
flow_tcf_send_nlcmd(struct mlx5_flow_tcf_context *tcf,
struct tcf_nlcb_context *ctx)
{
- struct tcf_nlcb_buf *bc, *bn;
- struct nlmsghdr *nlh;
+ struct tcf_nlcb_buf *bc = LIST_FIRST(&ctx->nlbuf);
int ret = 0;
- bc = LIST_FIRST(&ctx->nlbuf);
while (bc) {
+ struct tcf_nlcb_buf *bn = LIST_NEXT(bc, next);
+ struct nlmsghdr *nlh;
+ uint32_t msg = 0;
int rc;
- bn = LIST_NEXT(bc, next);
- if (bc->size) {
- flow_tcf_setack_nlcmd(bc);
- nlh = (struct nlmsghdr *)&bc->msg;
- rc = flow_tcf_nl_ack(tcf, nlh, bc->size, NULL, NULL);
- if (rc && !ret)
- ret = rc;
+ while (msg < bc->size) {
+ /*
+ * Send Netlink commands from buffer in one by one
+ * fashion. If we send multiple rule deletion commands
+ * in one Netlink message and some error occurs it may
+ * cause multiple ACK error messages and break sequence
+ * numbers of Netlink communication, because we expect
+ * the only one ACK reply.
+ */
+ assert((bc->size - msg) >= sizeof(struct nlmsghdr));
+ nlh = (struct nlmsghdr *)&bc->msg[msg];
+ assert((bc->size - msg) >= nlh->nlmsg_len);
+ msg += nlh->nlmsg_len;
+ rc = flow_tcf_nl_ack(tcf, nlh, 0, NULL, NULL);
+ if (rc) {
+ DRV_LOG(WARNING,
+ "netlink: cleanup error %d", rc);
+ if (!ret)
+ ret = rc;
+ }
}
rte_free(bc);
bc = bn;