Re: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the Linux-rdma v19 base

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, October 11, 2018 5:52 PM, Slava Ovsiienko:
> Subject: RE: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the
> Linux-rdma v19 base
> 
> > -----Original Message-----
> > From: Yongseok Koh
> > Sent: Thursday, October 4, 2018 2:48
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH] net/mlx5: flow counters support on the
> > Linux-rdma v19 base
> >
> > On Wed, Oct 03, 2018 at 03:29:12PM +0000, Slava Ovsiienko wrote:
> > > Mellanox mlx5 PMD supports Flow Counters via Verbs library.
> > > The current implementation is based on the Mellanox proprietary
> > > Verbs library included in MLNX OFED packages. The Flow Counter
> > > support is recently added into linux-rdma release (v19), so the mlx5
> > > PMD update is needed to provide Counter feature on the base of linux-
> rdma.
> > >
> > > mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+ and
> > provide
> > > flow counters for both.
> > >
> > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/Makefile          | 10 ++++++++
> > >  drivers/net/mlx5/mlx5.c            |  6 +++++
> > >  drivers/net/mlx5/mlx5_flow.c       |  9 ++++++-
> > >  drivers/net/mlx5/mlx5_flow.h       |  4 +++
> > >  drivers/net/mlx5/mlx5_flow_verbs.c | 52
> > ++++++++++++++++++++++++++++++--------
> > >  drivers/net/mlx5/mlx5_glue.c       | 41
> > ++++++++++++++++++++++++++++++
> > >  drivers/net/mlx5/mlx5_glue.h       | 16 ++++++++++++
> > >  7 files changed, 127 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> > > index ca1de9f..e3d2156 100644
> > > --- a/drivers/net/mlx5/Makefile
> > > +++ b/drivers/net/mlx5/Makefile
> > > @@ -162,6 +162,16 @@ mlx5_autoconf.h.new:
> > $(RTE_SDK)/buildtools/auto-config-h.sh
> > >  		type 'struct ibv_counter_set_init_attr' \
> > >  		$(AUTOCONF_OUTPUT)
> > >  	$Q sh -- '$<' '$@' \
> > > +		HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> > > +		infiniband/verbs.h \
> > > +		type 'struct ibv_counters_init_attr' \
> > > +		$(AUTOCONF_OUTPUT)
> > > +	$Q sh -- '$<' '$@' \
> > > +		HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 \
> > > +		infiniband/verbs.h \
> > > +		type 'struct ibv_counters_init_attr' \
> > > +		$(AUTOCONF_OUTPUT)
> > > +	$Q sh -- '$<' '$@' \
> >
> > I still don't understand what is different between the two. These are
> > exactly same checking, then why do you need to have two different
> > macros? From this script, HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT is
> same
> > as HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45, isn't it?
> 
> We have three options:
> - no counter support in kernel at all
> - "old" counter support (ibv_counter_set_init_attr is defined in verbs.h)
> - "new" counter support (ibv_counters_init_attr  is defined in verbs.h)
> 
> Three options require at least two compilations flags. The meanings are
> chosen:
> HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT means there is counter
> support (of any type)
> HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45 differentiates the
> support type
> 
> This approach allows to avoid clumsy constructions in code like this:
> #if __defined(HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \
> || __defined(HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45)

This is not needed anyway. 
The "old counters" exists only in MLNX_OFED not upstream. 
Once the "new counters" were implemented upstream the next OFED completely replaced the old implementation. 
Meaning, there is no driver having them both. 

Hence, we can avoid this complex construction and just hold a single macro flag for the new counters.