Re: [dpdk-dev] [PATCH 2/4] net/mlx5: fix Netlink communication routine

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Tuesday, November 13, 2018 15:22
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Yongseok Koh
> <yskoh@mellanox.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 2/4] net/mlx5: fix Netlink communication routine
> 
> Monday, November 12, 2018 10:02 PM, Slava Ovsiienko:
> > Subject: [PATCH 2/4] net/mlx5: fix Netlink communication routine
> >
> > While receiving the Netlink reply messages we should stop at DONE or
> > ACK message. The existing implementation stops at DONE message or if
> > no multiple message flag set ( NLM_F_MULTI). It prevents the single
> > query requests from working, these requests send the single reply
> > message without multi-message flag followed by ACK message. This patch
> > fixes receiving part of Netlink communication routine.
> >
> > Fixes: 6e74990b3463 ("net/mlx5: update E-Switch VXLAN netlink
> > routines")
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5_flow_tcf.c | 58 +++++++++++++++++++++++++-----
> > ----------
> >  1 file changed, 37 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c
> > b/drivers/net/mlx5/mlx5_flow_tcf.c
> > index 5a38940..4d154b6 100644
> > --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> > +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> > @@ -3732,44 +3732,60 @@ struct pedit_parser {  {
> >  	unsigned int portid = mnl_socket_get_portid(tcf->nl);
> >  	uint32_t seq = tcf->seq++;
> > -	int err, ret;
> > +	int ret, err = 0;
> >
> >  	assert(tcf->nl);
> >  	assert(tcf->buf);
> > -	if (!seq)
> > +	if (!seq) {
> >  		/* seq 0 is reserved for kernel event-driven notifications. */
> >  		seq = tcf->seq++;
> > +	}
> >  	nlh->nlmsg_seq = seq;
> >  	nlh->nlmsg_flags |= NLM_F_ACK;
> >  	ret = mnl_socket_sendto(tcf->nl, nlh, nlh->nlmsg_len);
> > -	err = (ret <= 0) ? errno : 0;
> > +	if (ret <= 0) {
> > +		/* Message send error occurres. */
> > +		rte_errno = errno;
> > +		return -rte_errno;
> > +	}
> >  	nlh = (struct nlmsghdr *)(tcf->buf);
> >  	/*
> >  	 * The following loop postpones non-fatal errors until multipart
> >  	 * messages are complete.
> >  	 */
> > -	if (ret > 0)
> > -		while (true) {
> > -			ret = mnl_socket_recvfrom(tcf->nl, tcf->buf,
> > -						  tcf->buf_size);
> > +	while (true) {
> 
> This while(true) is a bit disturbing (I know it exists also before).
> 
> Can't we bound it to some limit, e.g. is there any multiply of tcf->buf_size
> that if we receive more than that it is for sure an error?
> 
We receive the reply messages in the loop - one for each iteration.
Yes, it is expected the each message is smaller than tcf->buf_size, but the number
of messages is not limited theoretically. We query the dump, dump can be sent
by kernel in any number of reply messages, depending on size of dump, we can't
make any assumptions regarding the number of reply messages, we should receive
all of them until DONE or ACK message is encountered.

WBR, Slava
> > +		ret = mnl_socket_recvfrom(tcf->nl, tcf->buf, tcf->buf_size);
> > +		if (ret < 0) {
> > +			err = errno;
> > +			/*
> > +			 * In case of overflow Will receive till
> > +			 * end of multipart message. We may lost part
> > +			 * of reply messages but mark and return an error.
> > +			 */
> > +			if (err != ENOSPC ||
> > +			    !(nlh->nlmsg_flags & NLM_F_MULTI) ||
> > +			    nlh->nlmsg_type == NLMSG_DONE)
> > +				break;
> > +		} else {
> > +			ret = mnl_cb_run(nlh, ret, seq, portid, cb, arg);
> > +			if (!ret) {
> > +				/*
> > +				 * libmnl returns 0 if DONE or
> > +				 * success ACK message found.
> > +				 */
> > +				break;
> > +			}
> >  			if (ret < 0) {
> > +				/*
> > +				 * ACK message with error found
> > +				 * or some error occurred.
> > +				 */
> >  				err = errno;
> > -				if (err != ENOSPC)
> > -					break;
> > -			}
> > -			if (!err) {
> > -				ret = mnl_cb_run(nlh, ret, seq, portid,
> > -						 cb, arg);
> > -				if (ret < 0) {
> > -					err = errno;
> > -					break;
> > -				}
> > -			}
> > -			/* Will receive till end of multipart message */
> > -			if (!(nlh->nlmsg_flags & NLM_F_MULTI) ||
> > -			      nlh->nlmsg_type == NLMSG_DONE)
> >  				break;
> > +			}
> > +			/* We should continue receiving. */
> >  		}
> > +	}
> >  	if (!err)
> >  		return 0;
> >  	rte_errno = err;
> > --
> > 1.8.3.1