From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id E4EBBA05D3 for ; Wed, 22 May 2019 13:01:47 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D174525A1; Wed, 22 May 2019 13:01:46 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 9681FA49 for ; Wed, 22 May 2019 13:01:44 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 May 2019 04:01:43 -0700 X-ExtLoop1: 1 Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga005.jf.intel.com with ESMTP; 22 May 2019 04:01:42 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX151.ger.corp.intel.com (163.33.192.59) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 22 May 2019 12:01:41 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.155]) by IRSMSX156.ger.corp.intel.com ([169.254.3.61]) with mapi id 14.03.0415.000; Wed, 22 May 2019 12:01:41 +0100 From: "Ananyev, Konstantin" To: "Ergin, Mesut A" , "Xing, Beilei" , "Zhang, Qi Z" CC: "dev@dpdk.org" , Thomas Monjalon , "Yigit, Ferruh" , Andrew Rybchenko Thread-Topic: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx Thread-Index: AQHVC5/nSk5Y6arBtUeKB6GUGZx4SqZzsq5AgACPK4CAAGgugIABE+KAgAAbAfCAAC4/gIAA83eA Date: Wed, 22 May 2019 11:01:40 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580161636EF4@irsmsx105.ger.corp.intel.com> References: <1557980885-183777-1-git-send-email-mesut.a.ergin@intel.com> <1557980885-183777-3-git-send-email-mesut.a.ergin@intel.com> <2601191342CEEE43887BDE71AB97725801616360D7@irsmsx105.ger.corp.intel.com> <3615B82CA151CF42A86EDDD9846A8B38C7A838E6@ORSMSX112.amr.corp.intel.com> <2601191342CEEE43887BDE71AB9772580161636445@irsmsx105.ger.corp.intel.com> <3615B82CA151CF42A86EDDD9846A8B38C7A86A60@ORSMSX112.amr.corp.intel.com> <2601191342CEEE43887BDE71AB9772580161636A0D@irsmsx105.ger.corp.intel.com> <3615B82CA151CF42A86EDDD9846A8B38C7A872FE@ORSMSX112.amr.corp.intel.com> In-Reply-To: <3615B82CA151CF42A86EDDD9846A8B38C7A872FE@ORSMSX112.amr.corp.intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjVmMWJkZjMtYzVlNy00Nzg5LTliOWQtMWIxZDk2ZjQ2YTk5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiSHBtdnZvRXJjK3RNRUZoR0preldFbmpTOFArTTNlYkxHWllZN0xHWjlRVEpFT04zOFRVdnBMXC9GYVM5ajV0RXMifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 2/3] net/i40e: add runtime option to disable vector rx X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > > > > > > > > > > > > > > Vector RX functions are not at feature parity with non-vector= ones and > > > > > > > currently the vector RX path is enabled by default. Hence, th= e only > > > > > > > option to force selection of non-vector variants and be able = to retain > > > > > > > functionality is to disable vector PMD globally at compile ti= me via the > > > > > > > config file option. > > > > > > > > > > > > > > This patch introduces a new runtime device config option name= d > > > > > > > disable-vec-rx to allow users to limit the driver to make a c= hoice 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=3D1 is speci= fied 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 (th= us 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 thi= s when the > > > > > first packet that can be offloaded arrives). The design in this p= atch gives > > > > > such applications a choice to restrict possible RX functions to n= on-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 t= he first > > > filter rule is parsed, and unset when the last rule is removed. > > > > Why is that, if you can define it at configuration stage, and RX functi= on > > selection is based on it? >=20 > I don't know why it was designed that way -- maybe maintainers know the > historical context. What I am trying to say - if particular feature (FD) enablement/disablement affects RX/TX function selection, then there should be an ability to enable= /disable that feature at configuration state (dev_config/queue_setup). Function to change value of that feature at runtime (after dev_start/queue_= start) should return an error if new value can't be supported with already selecte= d RX/TX function.=20 If that's is not the case, then it sounds like a bug/gap in i40e driver tha= t needs to be fixed. =20 >=20 > > > > > As there are likely > > > other features not yet supported by the vector path, it felt more app= ropriate > > > to have a generic flag for applications to not choose vector path. > > > > Once again - if some vector RX doesn't support particular requested at = config > > feature, it wouldn't be selected. If it is not the case, then it is a b= ug in > > rx function selection code and should be fixed, instead of introducing > > workaround flags. > > >=20 > In any case, this has to be tied to a command line option, either on the = application > side (i.e. OVS) > or DPDK devargs side, since we don't want to hard code it to limit > RX functions to non-vector ones application-wide at compile time. I am not forcing you to use hard-coded values, I am saying that we need to follow standard API to enable/disable HW/PMD features. If there is a problem in that API/implementation - we need to fix it, not introduce some hackery workarounds (new devargs or so). Obviously i40e RX function variations (vector/scalar/etc.)=20 are PMD specific details that user shouldn't know about. Let say tomorrow someone can add support for FD into i40e vector-RX, or completely new RX function will be introduced for i40e that will void current selection logic. Again i40e is not the only PMD that supports FD, so we need generic and wel= l defined way to enable/disable it.=20 >=20 > As far as I can see, passing FDIR configuration via the rte_eth_conf stru= ct: > struct rte_fdir_conf fdir_conf; /**< FDIR configuration. DEPRECATED */ > was deprecated. I suspect in favor of the late binding design mentioned, = but > again, I don't know the history on that. IMO, this made devargs a better = choice. Ok, then it looks like there is a flaw in ethdev level API that needs to be= fixed: We deprecated old way to request FD usage without introducing new one. CC-ing to ethdev maintainers - Guys is there a new way to request FD enablement, instead of deprecated fdi= r_config? Seems like not, unless I missed something obvious. If not, then we probably need either to re-deprecate fdir_config, or introd= uce some new method. My first thought would be to add new DEV_RX_OFFLOAD_* flag(s). Does it make sense? Konstantin >=20 > > > 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" vecto= rized > > path). > > > > I don't think that's the same thing. > > From my perspective 'use-latest-supported-vec' is to overcome HW limita= tions. > > >=20 > That's fine, we seem to have different opinions on them. To me, both are > runtime configuration options that let users select what RX functions the= y > prefer to use with their HW. >=20 > > In your case, as I can see, all you need is to declare to HW/PMD that y= ou plan > > to use FD HW capabilities and proper RX function will be selected autom= atically > > (pretty much as with other HW offloads, TX cksum, RX multi-seg, etc.). > > >=20 > Offload configurations you mentioned can be passed on to DPDK from the > application via offloads field in rte_eth_conf, FD cannot. >=20 > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Mesut Ali Ergin > > > > > > > --- > > > > > > > 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=3D1 > > > > > > > > > > > > > > +- ``Disable RX Vector Functions `` (default ``vector RX enab= led``) > > > > > > > + > > > > > > > + When config file option for the vector PMD is enabled, vec= tor RX > > > > functions > > > > > > may > > > > > > > + be the default choice of the driver at device initializati= on time, if > > certain > > > > > > > + conditions apply. However, vector RX functions may not alw= ays be at > > > > > > feature > > > > > > > + parity with non-vector ones. In order to allow users to ov= erride > > vector > > > > RX > > > > > > > + function selection behavior at runtime, the ``devargs`` pa= rameter > > > > > > > + ``disable-vec-rx`` is introduced, such that > > > > > > > + > > > > > > > + -w DBDF,disable-vec-rx=3D1 > > > > > > > + > > > > > > > + would force driver to limit its choices to non-vector RX f= unction > > variants > > > > for > > > > > > > + the device specified by the DBDF. > > > > > > > + > > > > > > > Vector RX Pre-conditions > > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > For Vector RX it is assumed that the number of descriptor ri= ngs 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[] =3D= { > > > > > > > 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[] =3D { > > > > > > > @@ -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 *k= ey, > > > > > > > + const char *value, > > > > > > > + void *opaque) > > > > > > > +{ > > > > > > > + struct i40e_adapter *ad; > > > > > > > + > > > > > > > + ad =3D (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 =3D 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 =3D > > > > > > > + I40E_DEV_PRIVATE_TO_ADAPTER(dev->data- > > >dev_private); > > > > > > > + struct rte_kvargs *kvlist; > > > > > > > + int kvargs_count; > > > > > > > + > > > > > > > + > > > > > > > + if (!dev->device->devargs) > > > > > > > + return 0; > > > > > > > + > > > > > > > + kvlist =3D rte_kvargs_parse(dev->device->devargs->args, > > valid_keys); > > > > > > > + if (!kvlist) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > + kvargs_count =3D 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 =3D true; > > > > > > > ad->tx_vec_allowed =3D 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 sho= uld > > also be > > > > > > > * removed. > > > > > > > @@ -12790,4 +12857,5 @@ > > > > RTE_PMD_REGISTER_PARAM_STRING(net_i40e, > > > > > > > ETH_I40E_FLOATING_VEB_LIST_ARG > > "=3D" > > > > > > > ETH_I40E_QUEUE_NUM_PER_VF_ARG > > > > > > "=3D1|2|4|8|16" > > > > > > > ETH_I40E_SUPPORT_MULTI_DRIVER "=3D1" > > > > > > > - ETH_I40E_USE_LATEST_VEC "=3D0|1"); > > > > > > > + ETH_I40E_USE_LATEST_VEC "=3D0|1" > > > > > > > + ETH_I40E_DISABLE_VEC_RX "=3D0|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, voi= d > > > > *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 =3D true; > > > > > > > ad->rx_vec_allowed =3D true; > > > > > > > + > > > > > > > + /* Check if users wanted to disable vector RX functions > > */ > > > > > > > + i40e_disable_vec_rx(dev); > > > > > > > + > > > > > > > dev->data->scattered_rx =3D use_scattered_rx; > > > > > > > if (use_def_burst_func) > > > > > > > ad->rx_bulk_alloc_allowed =3D false; > > > > > > > -- > > > > > > > 2.7.4