Re: [dpdk-dev] [PATCH 2/2] net/mlx5: add fine grain dynamic flag support

[Ori Kam <orika@mellanox.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 16, 2020 2:24 PM
> To: Ori Kam <orika@mellanox.com>; Matan Azrad <matan@mellanox.com>;
> Shahaf Shuler <shahafs@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] net/mlx5: add fine grain dynamic flag
> support
> 
> On 1/16/2020 12:05 PM, Ori Kam wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Wednesday, January 15, 2020 4:02 PM
> >> To: Ori Kam <orika@mellanox.com>; Matan Azrad
> <matan@mellanox.com>;
> >> Shahaf Shuler <shahafs@mellanox.com>; Slava Ovsiienko
> >> <viacheslavo@mellanox.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [PATCH 2/2] net/mlx5: add fine grain dynamic flag support
> >>
> >> On 1/13/2020 9:29 AM, Ori Kam wrote:
> >>> The inline feature is designed to save PCI bandwidth by copying some
> >>> of the data to the wqe. This feature if enabled works for all packets.
> >>>
> >>> In some cases when using external memory, the PCI bandwidth is not
> >>> relevant since the memory can be accessed by other means.
> >>>
> >>> This commit introduce the ability to control the inline with mbuf
> >>> granularity.
> >>>
> >>> In order to use this feature the application should register the field
> >>> name, and restart the port.
> >>>
> >>> Signed-off-by: Ori Kam <orika@mellanox.com>
> >>> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >>> ---
> >>>  drivers/net/mlx5/mlx5.c                   | 15 +++++++++++++++
> >>>  drivers/net/mlx5/mlx5_rxtx.c              |  2 ++
> >>>  drivers/net/mlx5/mlx5_rxtx.h              |  3 +++
> >>>  drivers/net/mlx5/mlx5_trigger.c           |  8 ++++++++
> >>>  drivers/net/mlx5/rte_pmd_mlx5.h           | 32
> >> +++++++++++++++++++++++++++++++
> >>>  drivers/net/mlx5/rte_pmd_mlx5_version.map |  7 +++++++
> >>>  6 files changed, 67 insertions(+)
> >>>  create mode 100644 drivers/net/mlx5/rte_pmd_mlx5.h
> >>>
> >>> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> >>> index 50960c9..27dbe27 100644
> >>> --- a/drivers/net/mlx5/mlx5.c
> >>> +++ b/drivers/net/mlx5/mlx5.c
> >>> @@ -46,6 +46,7 @@
> >>>  #include "mlx5_glue.h"
> >>>  #include "mlx5_mr.h"
> >>>  #include "mlx5_flow.h"
> >>> +#include "rte_pmd_mlx5.h"
> >>>
> >>>  /* Device parameter to enable RX completion queue compression. */
> >>>  #define MLX5_RXQ_CQE_COMP_EN "rxq_cqe_comp_en"
> >>> @@ -1988,6 +1989,20 @@ struct mlx5_flow_id_pool *
> >>>  	return ret;
> >>>  }
> >>>
> >>> +int
> >>> +rte_pmd_mlx5_get_dyn_flag_names(char *names[], uint16_t n)
> >>> +{
> >>
> >> Now this is a public API, it should validate the user input.
> >>
> > Will add validation to make sure names != NULL,
> >
> >>> +	static const char *const dynf_names[] = {
> >>> +		RTE_PMD_MLX5_FINE_GRANULARITY_INLINE,
> >>> +	};
> >>> +	int num = RTE_MIN(n, RTE_DIM(dynf_names));
> >>> +	int i;
> >>> +
> >>> +	for (i = 0; i < num; i++)
> >>> +		strcpy(names[i], dynf_names[i]);
> >>> +	return num;
> >>> +}
> >>> +
> >>>  /**
> >>>   * Check sibling device configurations.
> >>>   *
> >>> diff --git a/drivers/net/mlx5/mlx5_rxtx.c
> b/drivers/net/mlx5/mlx5_rxtx.c
> >>> index 67cafd1..aa6aa22 100644
> >>> --- a/drivers/net/mlx5/mlx5_rxtx.c
> >>> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> >>> @@ -126,6 +126,8 @@ enum mlx5_txcmp_code {
> >>>  uint8_t mlx5_cksum_table[1 << 10] __rte_cache_aligned;
> >>>  uint8_t mlx5_swp_types_table[1 << 10] __rte_cache_aligned;
> >>>
> >>> +uint64_t rte_net_mlx5_dynf_inline_mask;
> >>> +
> >>>  /**
> >>>   * Build a table to translate Rx completion flags to packet type.
> >>>   *
> >>> diff --git a/drivers/net/mlx5/mlx5_rxtx.h
> b/drivers/net/mlx5/mlx5_rxtx.h
> >>> index e362b4a..7c38c57 100644
> >>> --- a/drivers/net/mlx5/mlx5_rxtx.h
> >>> +++ b/drivers/net/mlx5/mlx5_rxtx.h
> >>> @@ -42,6 +42,9 @@
> >>>  /* Support tunnel matching. */
> >>>  #define MLX5_FLOW_TUNNEL 9
> >>>
> >>> +/* Mbuf dynamic flag offset for inline. */
> >>> +extern uint64_t rte_net_mlx5_dynf_inline_mask;
> >>> +
> >>>  struct mlx5_rxq_stats {
> >>>  #ifdef MLX5_PMD_SOFT_COUNTERS
> >>>  	uint64_t ipackets; /**< Total of successfully received packets. */
> >>> diff --git a/drivers/net/mlx5/mlx5_trigger.c
> >> b/drivers/net/mlx5/mlx5_trigger.c
> >>> index ab6937a..ab253b2 100644
> >>> --- a/drivers/net/mlx5/mlx5_trigger.c
> >>> +++ b/drivers/net/mlx5/mlx5_trigger.c
> >>> @@ -13,6 +13,7 @@
> >>>  #include "mlx5.h"
> >>>  #include "mlx5_rxtx.h"
> >>>  #include "mlx5_utils.h"
> >>> +#include "rte_pmd_mlx5.h"
> >>>
> >>>  /**
> >>>   * Stop traffic on Tx queues.
> >>> @@ -270,8 +271,15 @@
> >>>  {
> >>>  	struct mlx5_priv *priv = dev->data->dev_private;
> >>>  	int ret;
> >>> +	int fine_inline;
> >>>
> >>>  	DRV_LOG(DEBUG, "port %u starting device", dev->data->port_id);
> >>> +	fine_inline = rte_mbuf_dynflag_lookup
> >>> +		(RTE_PMD_MLX5_FINE_GRANULARITY_INLINE, NULL);
> >>> +	if (fine_inline > 0)
> >>> +		rte_net_mlx5_dynf_inline_mask = 1UL << fine_inline;
> >>> +	else
> >>> +		rte_net_mlx5_dynf_inline_mask = 0;
> >>>  	ret = mlx5_dev_configure_rss_reta(dev);
> >>>  	if (ret) {
> >>>  		DRV_LOG(ERR, "port %u reta config failed: %s",
> >>> diff --git a/drivers/net/mlx5/rte_pmd_mlx5.h
> >> b/drivers/net/mlx5/rte_pmd_mlx5.h
> >>> new file mode 100644
> >>> index 0000000..12e18ca
> >>> --- /dev/null
> >>> +++ b/drivers/net/mlx5/rte_pmd_mlx5.h
> >>> @@ -0,0 +1,32 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>> + * Copyright 2020 Mellanox Technologies, Ltd
> >>> + */
> >>> +
> >>> +#ifndef RTE_PMD_PRIVATE_MLX5_H_
> >>> +#define RTE_PMD_PRIVATE_MLX5_H_
> >>> +
> >>> +/**
> >>> + * @file
> >>> + * MLX5 public header.
> >>> + *
> >>> + * This interface provides the ability to support private PMD
> >>> + * dynamic flags.
> >>> + */
> >>> +
> >>> +#define RTE_PMD_MLX5_FINE_GRANULARITY_INLINE
> >> "mlx5_fine_granularity_inline"
> >>> +
> >>> +/**
> >>> + * Returns the dynamic flags name, that are supported.
> >>> + *
> >>> + * @param[out] names
> >>> + *   Array that is used to return the supported dynamic flags names.
> >>> + * @param[in] n
> >>> + *   The number of elements in the names array.
> >>> + *
> >>> + * @return
> >>> + *   The number of dynamic flags that were copied.
> >>> + */
> >>> +__rte_experimental
> >>> +int rte_pmd_mlx5_get_dyn_flag_names(char *names[], uint16_t n);
> >>
> >> Can you please add this header to the API documentation index,
> >> doc/api/doxy-api-index.md, so it will be part of API document.
> >>
> >
> > Will add.
> >
> >>> +
> >>> +#endif
> >>> diff --git a/drivers/net/mlx5/rte_pmd_mlx5_version.map
> >> b/drivers/net/mlx5/rte_pmd_mlx5_version.map
> >>> index f9f17e4..c8b1031 100644
> >>> --- a/drivers/net/mlx5/rte_pmd_mlx5_version.map
> >>> +++ b/drivers/net/mlx5/rte_pmd_mlx5_version.map
> >>> @@ -1,3 +1,10 @@
> >>>  DPDK_20.0 {
> >>>  	local: *;
> >>>  };
> >>> +
> >>> +EXPERIMENTAL {
> >>> +        global:
> >>> +
> >>> +        # added in 20.02
> >>> +	rte_pmd_mlx5_get_dyn_flag_names;
> >>> +};
> >>>
> >>
> >> Isn't the datapath implementation missing? Where this new mbuf
> dynamic
> >> flag set
> >> or checked?
> >
> > The data path implementation will be done in different patch.
> > The flags is set for example using the testpmd new API or by the
> application.
> > The reason that I added this patch is to show usage for the testpmd patch, I
> can remove this patch and
> > re send it when sending the datapath patch.
> > What do you think?
> >
> 
> I think testpmd patch is clear enough on its own and better to send PMD
> patch
> separately when it is complete.

Will remove this patch.

Thanks,
Ori