On Tue, Aug 28, 2018 at 01:42:18PM -0700, Yongseok Koh wrote:
> On Fri, Aug 24, 2018 at 02:45:00PM +0800, Jack MIN wrote:
> > On Thu, Aug 23, 2018 at 02:08:09PM -0700, Yongseok Koh wrote:
> > > On Thu, Aug 23, 2018 at 02:38:51PM +0800, Xiaoyu Min wrote:
> > > > rte_errno is a per thread variable and is widely used as an
> > > > error indicator, which means a function could affect
> > > > other functions' results by setting rte_errno carelessly
> > > >
> > > > During rxq setup, an EINVAL rte_errno is expected since
> > > > the queues are not created yet
> > > > So rte_errno is cleared when it is EINVAL as expected
> > > >
> > > > Signed-off-by: Xiaoyu Min <jack...@mellanox.com>
> > > > ---
> > > > drivers/net/mlx5/mlx5_rxq.c | 20 +++++++++++++++-----
> > > > 1 file changed, 15 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> > > > index 1f7bfd4..e7056e8 100644
> > > > --- a/drivers/net/mlx5/mlx5_rxq.c
> > > > +++ b/drivers/net/mlx5/mlx5_rxq.c
> > > > @@ -443,6 +443,7 @@
> > > > struct mlx5_rxq_data *rxq = (*priv->rxqs)[idx];
> > > > struct mlx5_rxq_ctrl *rxq_ctrl =
> > > > container_of(rxq, struct mlx5_rxq_ctrl, rxq);
> > > > + int ret = 0;
> > > >
> > > > if (!rte_is_power_of_2(desc)) {
> > > > desc = 1 << log2above(desc);
> > > > @@ -459,13 +460,21 @@
> > > > rte_errno = EOVERFLOW;
> > > > return -rte_errno;
> > > > }
> > > > - if (!mlx5_rxq_releasable(dev, idx)) {
> > > > + ret = mlx5_rxq_releasable(dev, idx);
> > > > + if (!ret) {
> > > > DRV_LOG(ERR, "port %u unable to release queue index %u",
> > > > dev->data->port_id, idx);
> > > > rte_errno = EBUSY;
> > > > return -rte_errno;
> > > > + } else if (ret == -EINVAL) {
> > > > + /**
> > > > + * on the first time, rx queue doesn't exist,
> > > > + * so just ignore this error and reset rte_errno.
> > > > + */
> > > > + rte_errno = 0;
> > >
> > > Unless this function returns failure, the rte_errno will be ignored by
> > > caller
> > > and caller shouldn't assume rte_errno has 0. Caller will assume it is
> > > garbage
> > > data in case of success. So we can silently ignore this case. Does it
> > > cause any
> > > issue in application side?
> > >
> > Not application side but mlx5 PMD this time:
> > **mlx5_fdir_filter_delete**
> > which just _return -rte_errno;_
>
> Looks like an error. mlx5_fdir_filter_delete() can't be like that. We seem to
> have lost the code while refactoring it. Let take it offline.
>
Sure ~
> > For sure, _mlx5_fdir_filter_delete_ should be more defensive, should not
> > assume
> > rte_errno is zero if no error happened.
> > However if the caller know that an error will happen and rte_errno will
> > become
> > meaningless (garbage) for the succeeding functions, Catching the expected
> > error
> > and resetting rte_errno will be better. What do you think?
>
> Still don't understand clearly. There would be many other similar cases where
> we
> don't clear rte_errno when returning success. I don't understand why this case
> should be taken as a special one??
>
No, the _mlx5_rxq_releasable()_ returns *error* (-EINVAL)(the rxq doesn't exist)
as my understanding
We only check if rxq is not releasable(==0) in previouse version but we didn't
check if function returns error or success
> Thanks
> Yongseok
