> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, May 20, 2019 4:11 PM
> To: Ergin, Mesut A <mesut.a.er...@intel.com>; Xing, Beilei
> <beilei.x...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable
> vector rx
>
>
>
> > >
> > > Hi
> > >
> > > >
> > > > Vector RX functions are not at feature parity with non-vector ones and
> > > > currently the vector RX path is enabled by default. Hence, the only
> > > > option to force selection of non-vector variants and be able to retain
> > > > functionality is to disable vector PMD globally at compile time via the
> > > > config file option.
> > > >
> > > > This patch introduces a new runtime device config option named
> > > > disable-vec-rx to allow users to limit the driver to make a choice among
> > > > non-vector RX functions, on a per device basis. This runtime option
> > > > defaults to a value of zero, allowing vector RX functions to be
> > > > selected (current behavior). When disable-vec-rx=1 is specified for a
> > > > device, even if all the other requirements to select a vector RX
> > > > function are satisfied, the driver will still pick one out of the
> > > > appropriate non-vector RX functions.
> > >
> > > Not sure I understand the logic of that patch.
> > > If i40e RX PMD selects wrong RX function that doesn't provide
> > > requested by user functionality (which one?) -
> > > then it is a bug in i40e RX function selection code that needs to be
> > > fixed.
> > > No point to introduce some new config options for that.
> > > Konstantin
> > >
> > Current design of RX function selection for the PMDs make it an
> > initialization time deal. However, the first rte_flow request (thus the need
> > to enable FD / RTE_FDIR_MODE_PERFECT) may come in at any point in
> > time, well after the RX function was selected (e.g., OVS does this when the
> > first packet that can be offloaded arrives). The design in this patch gives
> > such applications a choice to restrict possible RX functions to non-vector
> > paths proactively.
>
> If you plan to use FD mode on your device, why not enable it
> at setup phase via rte_eth_dev_configure()?
> Then proper rx function would be selected.
>
FDIR_MODE was designed to bind late automatically -- it is set when the first
filter rule is parsed, and unset when the last rule is removed. As there are
likely
other features not yet supported by the vector path, it felt more appropriate
to have a generic flag for applications to not choose vector path. In fact, the
proposed option is the dual of already shipping "use-latest-supported-vec"
flag that lets the apps do the opposite (i.e. choose the "most" vectorized
path).
> >
> > > >
> > > > Signed-off-by: Mesut Ali Ergin <mesut.a.er...@intel.com>
> > > > ---
> > > > doc/guides/nics/i40e.rst | 14 +++++++++
> > > > drivers/net/i40e/i40e_ethdev.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++-
> > > > drivers/net/i40e/i40e_ethdev.h | 1 +
> > > > drivers/net/i40e/i40e_rxtx.c | 4 +++
> > > > 4 files changed, 88 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
> > > > index a97484c..532cc64 100644
> > > > --- a/doc/guides/nics/i40e.rst
> > > > +++ b/doc/guides/nics/i40e.rst
> > > > @@ -183,6 +183,20 @@ Runtime Config Options
> > > >
> > > > -w 84:00.0,use-latest-supported-vec=1
> > > >
> > > > +- ``Disable RX Vector Functions `` (default ``vector RX enabled``)
> > > > +
> > > > + When config file option for the vector PMD is enabled, vector RX
> functions
> > > may
> > > > + be the default choice of the driver at device initialization time,
> > > > if certain
> > > > + conditions apply. However, vector RX functions may not always be at
> > > feature
> > > > + parity with non-vector ones. In order to allow users to override
> > > > vector
> RX
> > > > + function selection behavior at runtime, the ``devargs`` parameter
> > > > + ``disable-vec-rx`` is introduced, such that
> > > > +
> > > > + -w DBDF,disable-vec-rx=1
> > > > +
> > > > + would force driver to limit its choices to non-vector RX function
> > > > variants
> for
> > > > + the device specified by the DBDF.
> > > > +
> > > > Vector RX Pre-conditions
> > > > ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > For Vector RX it is assumed that the number of descriptor rings will
> > > > be a
> power
> > > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> b/drivers/net/i40e/i40e_ethdev.c
> > > > index cab440f..19fbd23 100644
> > > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > > @@ -44,6 +44,7 @@
> > > > #define ETH_I40E_SUPPORT_MULTI_DRIVER "support-multi-driver"
> > > > #define ETH_I40E_QUEUE_NUM_PER_VF_ARG "queue-num-per-vf"
> > > > #define ETH_I40E_USE_LATEST_VEC "use-latest-supported-vec"
> > > > +#define ETH_I40E_DISABLE_VEC_RX "disable-vec-rx"
> > > >
> > > > #define I40E_CLEAR_PXE_WAIT_MS 200
> > > >
> > > > @@ -410,6 +411,7 @@ static const char *const valid_keys[] = {
> > > > ETH_I40E_SUPPORT_MULTI_DRIVER,
> > > > ETH_I40E_QUEUE_NUM_PER_VF_ARG,
> > > > ETH_I40E_USE_LATEST_VEC,
> > > > + ETH_I40E_DISABLE_VEC_RX,
> > > > NULL};
> > > >
> > > > static const struct rte_pci_id pci_id_i40e_map[] = {
> > > > @@ -1263,6 +1265,68 @@ i40e_use_latest_vec(struct rte_eth_dev *dev)
> > > > return 0;
> > > > }
> > > >
> > > > +static int
> > > > +i40e_parse_disable_vec_rx_handler(__rte_unused const char *key,
> > > > + const char *value,
> > > > + void *opaque)
> > > > +{
> > > > + struct i40e_adapter *ad;
> > > > +
> > > > + ad = (struct i40e_adapter *)opaque;
> > > > +
> > > > + switch (atoi(value)) {
> > > > + case 0:
> > > > + /* Selection of RX vector functions left untouched*/
> > > > + break;
> > > > + case 1:
> > > > + /* Disable RX vector functions as requested*/
> > > > + ad->rx_vec_allowed = false;
> > > > + break;
> > > > + default:
> > > > + PMD_DRV_LOG(WARNING, "Value should be 0 or 1, set it as
> > > 1!");
> > > > + break;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int
> > > > +i40e_disable_vec_rx(struct rte_eth_dev *dev)
> > > > +{
> > > > + struct i40e_adapter *ad =
> > > > + I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> > > > + struct rte_kvargs *kvlist;
> > > > + int kvargs_count;
> > > > +
> > > > +
> > > > + if (!dev->device->devargs)
> > > > + return 0;
> > > > +
> > > > + kvlist = rte_kvargs_parse(dev->device->devargs->args,
> > > > valid_keys);
> > > > + if (!kvlist)
> > > > + return -EINVAL;
> > > > +
> > > > + kvargs_count = rte_kvargs_count(kvlist,
> > > > ETH_I40E_DISABLE_VEC_RX);
> > > > + if (!kvargs_count) {
> > > > + rte_kvargs_free(kvlist);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + if (kvargs_count > 1)
> > > > + PMD_DRV_LOG(WARNING, "More than one argument \"%s\"
> > > and only "
> > > > + "the first invalid or last valid one is
> > > > used !",
> > > > + ETH_I40E_DISABLE_VEC_RX);
> > > > +
> > > > + if (rte_kvargs_process(kvlist, ETH_I40E_DISABLE_VEC_RX,
> > > > + i40e_parse_disable_vec_rx_handler, ad)
> > > > < 0) {
> > > > + rte_kvargs_free(kvlist);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + rte_kvargs_free(kvlist);
> > > > + return 0;
> > > > +}
> > > > +
> > > > #define I40E_ALARM_INTERVAL 50000 /* us */
> > > >
> > > > static int
> > > > @@ -1795,6 +1859,9 @@ i40e_dev_configure(struct rte_eth_dev *dev)
> > > > ad->tx_simple_allowed = true;
> > > > ad->tx_vec_allowed = true;
> > > >
> > > > + /* Check if users wanted to disable vector RX functions */
> > > > + i40e_disable_vec_rx(dev);
> > > > +
> > > > /* Only legacy filter API needs the following fdir config. So
> > > > when the
> > > > * legacy filter API is deprecated, the following codes should
> > > > also be
> > > > * removed.
> > > > @@ -12790,4 +12857,5 @@
> RTE_PMD_REGISTER_PARAM_STRING(net_i40e,
> > > > ETH_I40E_FLOATING_VEB_LIST_ARG "=<string>"
> > > > ETH_I40E_QUEUE_NUM_PER_VF_ARG
> > > "=1|2|4|8|16"
> > > > ETH_I40E_SUPPORT_MULTI_DRIVER "=1"
> > > > - ETH_I40E_USE_LATEST_VEC "=0|1");
> > > > + ETH_I40E_USE_LATEST_VEC "=0|1"
> > > > + ETH_I40E_DISABLE_VEC_RX "=0|1");
> > > > diff --git a/drivers/net/i40e/i40e_ethdev.h
> b/drivers/net/i40e/i40e_ethdev.h
> > > > index 9855038..906bfe9 100644
> > > > --- a/drivers/net/i40e/i40e_ethdev.h
> > > > +++ b/drivers/net/i40e/i40e_ethdev.h
> > > > @@ -1248,6 +1248,7 @@ int i40e_config_rss_filter(struct i40e_pf *pf,
> > > > struct i40e_rte_flow_rss_conf *conf, bool add);
> > > > int i40e_vf_representor_init(struct rte_eth_dev *ethdev, void
> *init_params);
> > > > int i40e_vf_representor_uninit(struct rte_eth_dev *ethdev);
> > > > +int i40e_disable_vec_rx(struct rte_eth_dev *dev);
> > > >
> > > > #define I40E_DEV_TO_PCI(eth_dev) \
> > > > RTE_DEV_TO_PCI((eth_dev)->device)
> > > > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > > index 1489552..7e66f59 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -1736,6 +1736,10 @@ i40e_dev_rx_queue_setup_runtime(struct
> > > rte_eth_dev *dev,
> > > > */
> > > > ad->rx_bulk_alloc_allowed = true;
> > > > ad->rx_vec_allowed = true;
> > > > +
> > > > + /* Check if users wanted to disable vector RX functions
> > > > */
> > > > + i40e_disable_vec_rx(dev);
> > > > +
> > > > dev->data->scattered_rx = use_scattered_rx;
> > > > if (use_def_burst_func)
> > > > ad->rx_bulk_alloc_allowed = false;
> > > > --
> > > > 2.7.4
