Re: [dpdk-dev] [PATCH 3/3] net/mlx5: fix rule cleanup Netlink command sending

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----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
> >