Re: [dpdk-dev] [RFC PATCH] build: allow passing extra config header to build
> -Original Message-
> From: dev On Behalf Of Bruce Richardson
> Sent: Thursday, November 12, 2020 11:32 AM
> To: dev@dpdk.org
> Cc: Bruce Richardson
> Subject: [dpdk-dev] [RFC PATCH] build: allow passing extra config header
> to build
>
> To allow per-build override of some settings, without having to change
> DPDK source-code files, i.e. rte_config.h, we can add an option to allow
> the user to pass in a file containing their own defines for the build.
>
This is definitely better than what we have now.
Even going forward with this as a temp solution would be good while we work out
longer-term strategy would be good.
What I believe is missing in the existing code base:
- knowing what all the knobs are
- ability to tune knobs in a build specific manner (providing per build config)
Regards,
Dave
> Signed-off-by: Bruce Richardson
> ---
> This is just a quick RFC to show what might be possible and to help keep
> the discussion going on how to improve build config in DPDK!
> ---
> config/meson.build | 10 ++
> config/rte_config.h | 4
> meson_options.txt | 2 ++
> 3 files changed, 16 insertions(+)
>
> diff --git a/config/meson.build b/config/meson.build index
> 258b01d06..5c137e4f5 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -14,6 +14,16 @@ foreach env:supported_exec_envs
> set_variable('is_' + env, exec_env == env) endforeach
>
> +if get_option('extra_config') != ''
> + extra_config = files(get_option('extra_config'))
> + configure_file(copy: true,
> + input: extra_config,
> + output: 'rte_build_config_extra.h',
> + install_dir: join_paths(get_option('includedir'),
> + get_option('include_subdir_arch')))
> + dpdk_conf.set('RTE_BUILD_CONFIG_EXTRA', 1) endif
> +
> # MS linker requires special treatment.
> # TODO: use cc.get_linker_id() with Meson >= 0.54 is_ms_linker =
> is_windows and (cc.get_id() == 'clang') diff --git a/config/rte_config.h
> b/config/rte_config.h index 25219f04a..2f5ecf999 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -148,4 +148,8 @@
> #define RTE_LIBRTE_PMD_DLB2_SW_CREDIT_QUANTA 32 #define
> RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH 256
>
> +#ifdef RTE_BUILD_CONFIG_EXTRA
> +#include
> +#endif
> +
> #endif /* _RTE_CONFIG_H_ */
> diff --git a/meson_options.txt b/meson_options.txt index
> 9bf18ab6b..91319b0e4 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -2,6 +2,8 @@
>
> option('armv8_crypto_dir', type: 'string', value: '',
> description: 'path to the armv8_crypto library installation
> directory')
> +option('extra_config', type: 'string', value: '',
> + description: 'path to a header file with extra build defines. Will
> be
> +installed as rte_build_config_extra.h')
> option('disable_drivers', type: 'string', value: '',
> description: 'Comma-separated list of drivers to explicitly
> disable.') option('drivers_install_subdir', type: 'string', value:
> 'dpdk/pmds-',
> --
> 2.25.1
Re: [dpdk-dev] Eliminate config/rte_config.h?
> -Original Message- > From: Bruce Richardson > Sent: Thursday, November 12, 2020 11:33 AM > To: David Harton (dharton) > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] Eliminate config/rte_config.h? > > On Thu, Nov 12, 2020 at 03:48:20PM +, Bruce Richardson wrote: > > On Thu, Nov 12, 2020 at 03:39:10PM +, David Harton (dharton) wrote: > > > Hi folks, > > > > > > > > > > > > Been experimenting with DPDK 20.11 and the loss of the old "config" > > > hierarchy. > > > > > > > > > > > > I wonder if anyone considered eliminating config/rte_config.h in > > > favor of a config/dpdk_conf that can contain the default values at > > > the start of the file but can be overridden with values later in the > > > file similar to how the old config files worked? > > > > > > > > > > > > It just feels awkward to modify a versioned header file per build > > > especially if multiple builds want to share the same repo but with > > > different values specified in config/rte_config.h especially when > > > "rte_build_config.h" is included at the very beginning of > > > config/rte_config.h. > > > > > > > > > > > > Thoughts? > > > > > > Dave > > > > > > > I'd personally be open to replacing it if we have a better solution. > > Are there many values in it that you look to tune, A couple that quickly come to mind are RTE_LOG_DP_LEVEL and RTE_BACKTRACE; but, doesn't a single tunable value suggest a different mechanism is needed? > > because I believe > > the general consensus is that we want to reduce the number of tunables > > we have per DPDK build? Sounds good. But doesn't that imply the config knob (i.e. #define) should just be removed or minimally moved? IOW, things in rte_config.h should be tunable values and if they aren't tunable then they should like be in component specific "private" header files? > > To that end our main concern should be having > > decent defaults, and thereafter we can see about the best mechanism to > > allow easy overriding for edge-cases. > > > > Any early attempt at such a thing was tried [1], but unfortunately it > > had issues so we need a new approach - possibly one as you had > suggested. > > > Just sent another idea of what options might be possible: > http://patches.dpdk.org/patch/84068/ That would be better. What if we had a config/dpdk_conf with all the defaults defined using the old 'config' syntax and then tools just append any overrides as desired. A simple tool could parse config/dpdk_conf to generate the appropriate rte_config.h instead of rte_build_config.h? BTW, one of the reasons why I'm asking about a more "config" like approach to help ease switching between the make and meson build methods for platforms. It may also help folks adopt the meson approach more easily. Dave > > Regards, > /Bruce
[dpdk-dev] Eliminate config/rte_config.h?
Hi folks, Been experimenting with DPDK 20.11 and the loss of the old "config" hierarchy. I wonder if anyone considered eliminating config/rte_config.h in favor of a config/dpdk_conf that can contain the default values at the start of the file but can be overridden with values later in the file similar to how the old config files worked? It just feels awkward to modify a versioned header file per build especially if multiple builds want to share the same repo but with different values specified in config/rte_config.h especially when "rte_build_config.h" is included at the very beginning of config/rte_config.h. Thoughts? Dave
Re: [dpdk-dev] [PATCH v2] net/ixgbevf: update VF_STAT macros to handle rollover
> -Original Message-
> From: Ferruh Yigit
> Sent: Wednesday, January 29, 2020 6:23 AM
> To: David Harton (dharton) ; dev@dpdk.org
> Cc: wenzhuo...@intel.com; konstantin.anan...@intel.com;
> xiaolong...@intel.com; intel@cisco.com
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbevf: update VF_STAT macros to
> handle rollover
>
> On 1/26/2020 5:32 PM, David Harton wrote:
> > Added rollover logic to UPDATE_VF_STAT and UPDATE_VF_STAT_36BIT macros.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: intel.com
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 49285ce53..bc73ad195 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -385,7 +385,11 @@ static void ixgbe_l2_tunnel_conf(struct rte_eth_dev
> *dev);
> > #define UPDATE_VF_STAT(reg, last, cur) \
> > { \
> > uint32_t latest = IXGBE_READ_REG(hw, reg); \
> > - cur += (latest - last) & UINT_MAX; \
>
> Here since 'last' is 'u64', the 'UINT_MAX' is required, but overall this
> looks good, original code should be OK.
Agreed. As mentioned on the igbvf thread I've gone back to the developer for
justification.
Regards,
Dave
>
> > + if (latest >= last) \
> > + cur += (latest - last); \
> > + else\
> > + cur += ((latest + ((uint64_t)1 << 32)) - last); \
> > + cur &= UINT_MAX;\
> > last = latest; \
> > }
> >
> > @@ -394,7 +398,11 @@ static void ixgbe_l2_tunnel_conf(struct rte_eth_dev
> *dev);
> > u64 new_lsb = IXGBE_READ_REG(hw, lsb); \
> > u64 new_msb = IXGBE_READ_REG(hw, msb); \
> > u64 latest = ((new_msb << 32) | new_lsb);\
> > - cur += (0x10LL + latest - last) & 0xFLL; \
> > + if (latest >= last) \
> > + cur += (latest - last); \
> > + else \
> > + cur += ((latest + ((u64)1 << 36)) - last); \
> > + cur &= 0xFLL;\
>
> For this case old and new implementation looks same to me.
>
> > last = latest; \
> > }
> >
> >
Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to handle rollover
> -Original Message-
> From: Ferruh Yigit
> Sent: Wednesday, January 29, 2020 5:10 AM
> To: David Harton (dharton) ; dev@dpdk.org
> Cc: wenzhuo...@intel.com; konstantin.anan...@intel.com;
> xiaolong...@intel.com; intel@cisco.com
> Subject: Re: [dpdk-dev] [PATCH v2] net/e1000: update UPDATE_VF_STAT to
> handle rollover
>
> On 1/26/2020 5:25 PM, David Harton wrote:
> > Modified UPDATE_VF_STAT to properly handle rollover conditions.
> >
> > Fixes: d82170d27918 ("igb: add VF support")
> > Cc: intel.com
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/e1000/igb_ethdev.c | 14 +-
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/e1000/igb_ethdev.c
> > b/drivers/net/e1000/igb_ethdev.c index a3e30dbe5..825663267 100644
> > --- a/drivers/net/e1000/igb_ethdev.c
> > +++ b/drivers/net/e1000/igb_ethdev.c
> > @@ -261,11 +261,15 @@ static int igb_filter_restore(struct rte_eth_dev
> > *dev);
> > /*
> > * Define VF Stats MACRO for Non "cleared on read" register
> > */
> > -#define UPDATE_VF_STAT(reg, last, cur)\
> > -{ \
> > - u32 latest = E1000_READ_REG(hw, reg); \
> > - cur += (latest - last) & UINT_MAX;\
>
> Why this is wrong? Both 'latest' and 'last' are 'u32', so diff should be
> correct 'u32' value. And it is added to 'u64' 'cur' value. What I am
> missing?
>
> > - last = latest;\
> > +#define UPDATE_VF_STAT(reg, last, cur) \
> > +{ \
> > + u32 latest = E1000_READ_REG(hw, reg); \
> > + if (latest >= last) \
> > + cur += (latest - last); \
> > + else\
> > + cur += ((latest + ((uint64_t)1 << 32)) - last); \
> > + cur &= UINT_MAX;\
>
> Why & with UINT_MAX, won't this limit the value to 32bits which has 64bit
> storage?
I'm embarrassed. I was upstreaming this on behalf of another and honestly
didn't even look. :(
You are right about the '&='. In fact, I'm not convinced these diffs are
necessary and have asked our local developer to verify why this change and the
ixgbevf change are needed. I'm wondering if they were encountering another
issue related to sync that cause pkt counts to get out of sync as they were
attempting to fix what looked like a rollover issue (huge packet counts after
boot).
Sorry,
Dave
>
> > + last = latest; \
> > }
> >
> > #define IGB_FC_PAUSE_TIME 0x0680
> >
Re: [dpdk-dev] [PATCH] net/ena: initialize PCI info in eth_ena_dev_init
> -Original Message-
> From: Ferruh Yigit
> Sent: Thursday, December 12, 2019 9:54 AM
> To: David Harton (dharton) ; dev@dpdk.org;
> m...@semihalf.com; m...@semihalf.com; gtza...@amazon.com; evge...@amazon.com;
> igo...@amazon.com
> Subject: Re: [dpdk-dev] [PATCH] net/ena: initialize PCI info in
> eth_ena_dev_init
>
> On 12/10/2019 10:37 PM, David Harton wrote:
> > Network drivers were originally modified to initialize PCI info with
> > commit eeefe73f0af1 ("drivers: copy PCI device info to ethdev data").
> > Modifying net/ena to conform.
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ena/ena_ethdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ena/ena_ethdev.c
> > b/drivers/net/ena/ena_ethdev.c index 7d4a3b225..4170cf749 100644
> > --- a/drivers/net/ena/ena_ethdev.c
> > +++ b/drivers/net/ena/ena_ethdev.c
> > @@ -1701,6 +1701,7 @@ static int eth_ena_dev_init(struct rte_eth_dev
> *eth_dev)
> > adapter->rte_dev = eth_dev;
> >
> > pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> > + rte_eth_copy_pci_info(eth_dev, pci_dev);
> > adapter->pdev = pci_dev;
> >
> > PMD_INIT_LOG(INFO, "Initializing %x:%x:%x.%d",
> >
>
> Hi David,
>
> Is it really needed, since it should be called by API already [1], what
> field in eth_dev was missing?
Hi Ferruh,
Great catch! I'm embarrassed.
In all honesty, I made this fix for our applications back for DPDK 16.07. At
that time, the drv_name used to get initialized by rte_eth_copy_pci_info() and
each driver was responsible for calling rte_eth_copy_pci_info() in their init
functions. Our apps would crash for ena devices when we tried to reference the
drv_name.
I just realized I had never upstreamed this private patch and pushed it along
with some others recently made yesterday.
So, you're 100% right it is no longer needed. Probably all of the drivers
should remove calling it in their init functions to remove this
legacy/deprecated requirement.
Sorry for the noise...
Regards,
Dave
>
> [1]
> eth_ena_pci_probe()
> rte_eth_dev_pci_generic_probe(eth_ena_dev_init)
> rte_eth_dev_pci_allocate()
> rte_eth_copy_pci_info()
> eth_ena_dev_init()
Re: [dpdk-dev] [PATCH] net/ena: Fix admin cq polling for 32-bit apps
> -Original Message-
> From: Michał Krawczyk
> Sent: Monday, July 01, 2019 3:24 AM
> To: David Harton (dharton)
> Cc: dev@dpdk.org; Marcin Wojtas ; Tzalik, Guy
> ; Schmeilin, Evgeny ; Belgazal,
> Netanel ; Kiyanovski, Arthur ;
> Chauskin, Igor ; Matushevsky, Alexander
> ; same...@amazon.com
> Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps
>
> + folks responsible for ENA on other platforms as this code touches
> every ENA target
>
> pt., 28 cze 2019 o 17:46 David Harton (dharton)
> napisał(a):
> >
> >
> >
> > > -Original Message-
> > > From: Michał Krawczyk
> > > Sent: Friday, June 28, 2019 11:03 AM
> > > To: David Harton (dharton)
> > > Cc: dev@dpdk.org; Marcin Wojtas ; Tzalik, Guy
> > > ; Schmeilin, Evgeny
> > > Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps
> > >
> > > Hi,
> > >
> > > sorry for the late reply.
> > >
> > > śr., 29 maj 2019 o 23:01 David Harton napisał(a):
> > > >
> > > > Recent modifications to admin command queue polling logic did not
> > > > support 32-bit applications. Updated the driver to work for 32 or
> > > > 64 bit applications as well as avoiding roll-over possibility.
> > > >
> > > > Fixes: 3adcba9a89 ("net/ena: update HAL to the newer version")
> > > >
> > > > Signed-off-by: David Harton
> > > > ---
> > > > drivers/net/ena/base/ena_com.c | 10 +++---
> > > > drivers/net/ena/base/ena_plat_dpdk.h | 6 +-
> > > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ena/base/ena_com.c
> > > > b/drivers/net/ena/base/ena_com.c index b688067f7..b96adde3c 100644
> > > > --- a/drivers/net/ena/base/ena_com.c
> > > > +++ b/drivers/net/ena/base/ena_com.c
> > > > @@ -547,10 +547,13 @@ static int
> > > ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx
> > > *comp_c
> > > > struct
> > > > ena_com_admin_queue *admin_queue) {
> > > > unsigned long flags = 0;
> > > > - unsigned long timeout;
> > > > + u32 timeout_ms;
> > > > int ret;
> > > >
> > > > - timeout = ENA_GET_SYSTEM_TIMEOUT(admin_queue-
> > > >completion_timeout);
> > > > + /* Calculate ms granularity timeout from us
> completion_timeout
> > > > +* making sure we retry once if we have at least 1ms
> > > > +*/
> > > > + timeout_ms = (admin_queue->completion_timeout / 1000) +
> > > > + (ENA_POLL_MS - 1);
> > > >
> > > > while (1) {
> > > > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); @@
> > > > -560,7 +563,7 @@ static int
> > > ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx
> > > *comp_c
> > > > if (comp_ctx->status != ENA_CMD_SUBMITTED)
> > > > break;
> > > >
> > > > - if (ENA_TIME_EXPIRE(timeout)) {
> > > > + if (timeout_ms < ENA_POLL_MS) {
> > > > ena_trc_err("Wait for completion (polling)
> > > timeout\n");
> > > > /* ENA didn't have any completion */
> > > > ENA_SPINLOCK_LOCK(admin_queue->q_lock,
> > > > flags); @@ -573,6 +576,7 @@ static int
> > > ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx
> > > *comp_c
> > > > }
> > > >
> > > > ENA_MSLEEP(ENA_POLL_MS);
> > > > + timeout_ms -= ENA_POLL_MS;
> > >
> > > This part can be problematic at the very overloaded systems - in
> > > that case the ENA_MSLEEP can take a much longer than ENA_POLL_MS and
> > > in this situation the time spent in this function can't be determined.
> > > That's why we were checking time spent in sleep every
> > > ENA_TIME_EXPIRE macro.
> > > The issue can be observed especially in the kernel drivers, and
> > > ena_com is common file for all ENA drivers.
> >
> > I don't understand the comment/concern.
> >
> > The previous macros calculate the future cycle count based on a us
> timeout value (assuming 64 b
Re: [dpdk-dev] [PATCH] net/ena: Fix admin cq polling for 32-bit apps
> -Original Message-
> From: Michał Krawczyk
> Sent: Friday, June 28, 2019 11:03 AM
> To: David Harton (dharton)
> Cc: dev@dpdk.org; Marcin Wojtas ; Tzalik, Guy
> ; Schmeilin, Evgeny
> Subject: Re: [PATCH] net/ena: Fix admin cq polling for 32-bit apps
>
> Hi,
>
> sorry for the late reply.
>
> śr., 29 maj 2019 o 23:01 David Harton napisał(a):
> >
> > Recent modifications to admin command queue polling logic did not
> > support 32-bit applications. Updated the driver to work for 32 or 64
> > bit applications as well as avoiding roll-over possibility.
> >
> > Fixes: 3adcba9a89 ("net/ena: update HAL to the newer version")
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ena/base/ena_com.c | 10 +++---
> > drivers/net/ena/base/ena_plat_dpdk.h | 6 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ena/base/ena_com.c
> > b/drivers/net/ena/base/ena_com.c index b688067f7..b96adde3c 100644
> > --- a/drivers/net/ena/base/ena_com.c
> > +++ b/drivers/net/ena/base/ena_com.c
> > @@ -547,10 +547,13 @@ static int
> ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
> > struct
> > ena_com_admin_queue *admin_queue) {
> > unsigned long flags = 0;
> > - unsigned long timeout;
> > + u32 timeout_ms;
> > int ret;
> >
> > - timeout = ENA_GET_SYSTEM_TIMEOUT(admin_queue-
> >completion_timeout);
> > + /* Calculate ms granularity timeout from us completion_timeout
> > +* making sure we retry once if we have at least 1ms
> > +*/
> > + timeout_ms = (admin_queue->completion_timeout / 1000) +
> > + (ENA_POLL_MS - 1);
> >
> > while (1) {
> > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags); @@
> > -560,7 +563,7 @@ static int
> ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
> > if (comp_ctx->status != ENA_CMD_SUBMITTED)
> > break;
> >
> > - if (ENA_TIME_EXPIRE(timeout)) {
> > + if (timeout_ms < ENA_POLL_MS) {
> > ena_trc_err("Wait for completion (polling)
> timeout\n");
> > /* ENA didn't have any completion */
> > ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
> > @@ -573,6 +576,7 @@ static int
> ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
> > }
> >
> > ENA_MSLEEP(ENA_POLL_MS);
> > + timeout_ms -= ENA_POLL_MS;
>
> This part can be problematic at the very overloaded systems - in that case
> the ENA_MSLEEP can take a much longer than ENA_POLL_MS and in this
> situation the time spent in this function can't be determined.
> That's why we were checking time spent in sleep every ENA_TIME_EXPIRE
> macro.
> The issue can be observed especially in the kernel drivers, and ena_com is
> common file for all ENA drivers.
I don't understand the comment/concern.
The previous macros calculate the future cycle count based on a us timeout
value (assuming 64 bit apps) and repeat the loop until the command is
"submitted" or the current cycle count is greater than the calculated cycle
count value sleeping ENA_POLL_MS between each iteration.
The new method accomplishes the same thing but instead of using a "cycle count"
it uses the number of ms which the poll and sleep actions are based upon.
The differences with the new method are:
- it uses less instructions
- not susceptible to cycle count overrun (admittedly highyl unlikely)
- (most importantly) works equally well for 32 or 64 bit apps
Can you elaborate on your concern?
Thanks,
Dave
Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c
> -Original Message- > From: Ferruh Yigit > Sent: Tuesday, June 04, 2019 12:00 PM > To: Bruce Richardson ; David Harton (dharton) > > Cc: dev@dpdk.org; beilei.x...@intel.com; qi.z.zh...@intel.com > Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in > i40e_rxtx.c > > On 5/16/2019 3:08 PM, Bruce Richardson wrote: > > On Wed, May 15, 2019 at 12:13:46PM -0400, David Harton wrote: > >> Use of weak symbols can hide makefile errors especially when custom > >> makefiles are used. Removing the use of weak symbols to avoid a stub > >> function being linked in production code. > >> > >> Signed-off-by: David Harton > >> --- > >> > >> v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors > >> > > Testing a few compiles here, this breaks when vector mode is disabled, > > because it's possible that CC_AVX2_SUPPORT=1 when VECTOR=n. I'd > > suggest adding "ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ... > > endif" around the block in the makefile checking for AVX2 support, so > > that we never set AVX2 unless we have vector support. > > Concern is this is pushing vectorization support more to compile time > configuration. Do we really have to select if to use vector PMD or not in > compile time? > > Can't we get rid of the 'CONFIG_RTE_LIBRTE_I40E_INC_VECTOR' config option > completely? As done in the ICE driver now. > > Isn't it better to compile vectorization support in as much as possible > and do the vector or scalar path selection in runtime, this patch may > prevent us to do that, weak functions enables us being more dynamic. Choosing a vector dynamically can be done without the use of weak symbols. IMHO, weak symbols should really be used for cross library support where a user wants to override a 3rd party function not within a lib. In fact the weak symbol usage may preclude supporting all the variants you might need/want to select. > > > > > With this change, you can include my ack in v3. > > > > /Bruce > > > > Acked-by: Bruce Richardson > >
Re: [dpdk-dev] [PATCH] net/ixgbevf: remove MTU setting limitation
Hi,
> -Original Message-
> From: Zhao1, Wei
> Sent: Tuesday, April 02, 2019 10:26 PM
> To: David Harton (dharton) ; dev@dpdk.org
> Cc: Lu, Wenzhuo ; Ananyev, Konstantin
> ; Zhang, Qi Z
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbevf: remove MTU setting limitation
>
> HI,
>
> Why not add some more code
> "
> if (rx_conf->offloads & DEV_RX_OFFLOAD_SCATTER)
> dev->data->scattered_rx = 1;
> "
>
> Into ixgbevf_dev_rx_init() to enable scatter mode when start device?
I don't think it's necessary is it?
Doesn't this code in ixgbevf_dev_rx_init() handle it?
if (rxmode->offloads & DEV_RX_OFFLOAD_SCATTER ||
/* It adds dual VLAN length for supporting dual VLAN */
(rxmode->max_rx_pkt_len +
2 * IXGBE_VLAN_TAG_SIZE) > buf_size) {
if (!dev->data->scattered_rx)
PMD_INIT_LOG(DEBUG, "forcing scatter mode");
dev->data->scattered_rx = 1;
}
Regards,
Dave
>
>
> Reviewed-by: Wei Zhao
>
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of David Harton
> > Sent: Wednesday, April 3, 2019 9:19 AM
> > To: dev@dpdk.org
> > Cc: Lu, Wenzhuo ; Ananyev, Konstantin
> > ; David Harton
> > Subject: [dpdk-dev] [PATCH] net/ixgbevf: remove MTU setting limitation
> >
> > Currently, if requested MTU is bigger than mbuf size and scattered
> > receive is not enabled, setting MTU to that value fails.
> >
> > This patch allows setting this special MTU when device is stopped,
> > because scattered_rx will be re-configured during next port start and
> > driver may enable scattered receive according new MTU value.
> >
> > After this patch, driver may select different receive function
> > automatically after MTU set, according MTU values selected.
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 12 +++-
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index feec85634..0b962c8db 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -6346,20 +6346,22 @@ ixgbevf_dev_set_mtu(struct rte_eth_dev *dev,
> > uint16_t mtu) {
> > struct ixgbe_hw *hw;
> > uint32_t max_frame = mtu + IXGBE_ETH_OVERHEAD;
> > - struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
> > + struct rte_eth_dev_data *dev_data = dev->data;
> >
> > hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> > if ((mtu < ETHER_MIN_MTU) || (max_frame >
> > ETHER_MAX_JUMBO_FRAME_LEN))
> > return -EINVAL;
> >
> > - /* refuse mtu that requires the support of scattered packets when
> > this
> > -* feature has not been enabled before.
> > + /* If device is started, refuse mtu that requires the support of
> > +* scattered packets when this feature has not been enabled before.
> > */
> > - if (!(rx_conf->offloads & DEV_RX_OFFLOAD_SCATTER) &&
> > + if (dev_data->dev_started && !dev_data->scattered_rx &&
> > (max_frame + 2 * IXGBE_VLAN_TAG_SIZE >
> > -dev->data->min_rx_buf_size - RTE_PKTMBUF_HEADROOM))
> > +dev->data->min_rx_buf_size - RTE_PKTMBUF_HEADROOM)) {
> > + PMD_INIT_LOG(ERR, "Stop port first.");
> > return -EINVAL;
> > + }
> >
> > /*
> > * When supported by the underlying PF driver, use the
> > IXGBE_VF_SET_MTU
> > --
> > 2.19.1
Re: [dpdk-dev] [PATCH v2] net/ixgbe: Restore vlan filter/extend for ixgbevf
Hi Qi,
> -Original Message-
> From: Zhang, Qi Z
> Sent: Thursday, March 14, 2019 11:14 AM
> To: David Harton (dharton)
> Cc: Lu, Wenzhuo ; Ananyev, Konstantin
> ; dev@dpdk.org
> Subject: RE: [PATCH v2] net/ixgbe: Restore vlan filter/extend for ixgbevf
>
>
>
> > -Original Message-
> > From: David Harton (dharton) [mailto:dhar...@cisco.com]
> > Sent: Thursday, March 14, 2019 9:38 PM
> > To: David Harton (dharton) ; Zhang, Qi Z
> >
> > Cc: Lu, Wenzhuo ; Ananyev, Konstantin
> > ; dev@dpdk.org
> > Subject: RE: [PATCH v2] net/ixgbe: Restore vlan filter/extend for
> > ixgbevf
> >
> > Hi Qi,
> >
> > This patch was rejected with no feedback.
> >
> > Can you share why?
>
> Because as Zhao Wei explained in v1 thread, in ixgbe_vlan_offload_config ,
> ETH_VLAN_FILTER_MASK and DEV_RX_OFFLOAD_VLAN_EXTEND are ignored This looks
> like VLAN_FILTER AND VLAN_EXTEND offload is not supported and I didn't see
> you reply for that, so I assume you agree with that.
>
>
> >
> > Without this patch users are not able to call the
> > rte_eth_dev_vlan_filter() API for ixgbevf based ports because
> > VLAN_FILTER cannot be enabled when calling rte_eth_dev_configure().
>
> OK, seems vlan filter is able to be configured for ixgbevf, but I don't
> know how this will work because vlan filter table needs to be enabled
> first (ixgbe_vlan_hw_filter_enable) So at least when it work with a DPDK
> PF, vlan filter will not work, maybe kernel PF driver will enable it by
> default? Could you share more insight?
I can't answer that but ixgbevf_vlan_filter_set() has been implemented for
quite some time and it can no longer be called since the new offload flags are
being enforced now.
I can share that rte_eth_dev_vlan_filter()/ixgbevf_vlan_filter_set() works
properly with the propose change (VLAN traffic flows) where it doesn't without
it.
>
> And same question to VLAN_EXTEND
I actually don't care about extend but simply changed it because it seems to be
in the same situation. I'm happy to leave it out of the patch if that grants
acceptance.
Thanks,
Dave
>
>
> >
> > Or, am I missing something?
> >
> > Thanks,
> > Dave
> >
> > > -Original Message-
> > > From: David Harton
> > > Sent: Friday, March 08, 2019 3:35 PM
> > > To: dev@dpdk.org
> > > Cc: wenzhuo...@intel.com; konstantin.anan...@intel.com;
> > > qi.z.zh...@intel.com; David Harton (dharton)
> > > Subject: [PATCH v2] net/ixgbe: Restore vlan filter/extend for
> > > ixgbevf
> > >
> > > ixgbevf vlan strip and extend capabilities were removed when
> > > migrating to the bit flags implementation.
> > >
> > > Restoring the capbility to enable these offloads at configuration
> time.
> > >
> > > Fixes: ec3b1124d14d (\"net/ixgbe: convert to new Rx offloads API\")
> > > Signed-off-by: David Harton
> > > ---
> > >
> > > v2: removed unused function ixgbe_is_vf()
> > >
> > > drivers/net/ixgbe/ixgbe_rxtx.c | 23 ++-
> > > 1 file changed, 2 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > b/drivers/net/ixgbe/ixgbe_rxtx.c index e92a70fb3..b1b83613e 100644
> > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > > @@ -2813,23 +2813,6 @@ ixgbe_reset_rx_queue(struct ixgbe_adapter
> > > *adapter, struct ixgbe_rx_queue *rxq) #endif }
> > >
> > > -static int
> > > -ixgbe_is_vf(struct rte_eth_dev *dev) -{
> > > - struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> > > >dev_private);
> > > -
> > > - switch (hw->mac.type) {
> > > - case ixgbe_mac_82599_vf:
> > > - case ixgbe_mac_X540_vf:
> > > - case ixgbe_mac_X550_vf:
> > > - case ixgbe_mac_X550EM_x_vf:
> > > - case ixgbe_mac_X550EM_a_vf:
> > > - return 1;
> > > - default:
> > > - return 0;
> > > - }
> > > -}
> > > -
> > > uint64_t
> > > ixgbe_get_rx_queue_offloads(struct rte_eth_dev *dev) { @@ -2853,15
> > > +2836,13 @@ ixgbe_get_rx_port_offloads(struct rte_eth_dev *dev)
> > > DEV_RX_OFFLOAD_TCP_CKSUM |
> > > DEV_RX_OFFLOAD_KEEP_CRC|
> > > DEV_RX_OFFLOAD_JUMBO_FRAME |
> > > +DEV_RX_OFFLOAD_VLAN_FILTER |
> > > +DEV_RX_OFFLOAD_VLAN_EXTEND |
> > > DEV_RX_OFFLOAD_SCATTER;
> > >
> > > if (hw->mac.type == ixgbe_mac_82598EB)
> > > offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > >
> > > - if (ixgbe_is_vf(dev) == 0)
> > > - offloads |= (DEV_RX_OFFLOAD_VLAN_FILTER |
> > > - DEV_RX_OFFLOAD_VLAN_EXTEND);
> > > -
> > > /*
> > >* RSC is only supported by 82599 and x540 PF devices in a non-SR-
> > > IOV
> > >* mode.
> > > --
> > > 2.19.1
Re: [dpdk-dev] [PATCH v2] net/ixgbe: Restore vlan filter/extend for ixgbevf
Hi Qi,
This patch was rejected with no feedback.
Can you share why?
Without this patch users are not able to call the rte_eth_dev_vlan_filter() API
for ixgbevf based ports because VLAN_FILTER cannot be enabled when calling
rte_eth_dev_configure().
Or, am I missing something?
Thanks,
Dave
> -Original Message-
> From: David Harton
> Sent: Friday, March 08, 2019 3:35 PM
> To: dev@dpdk.org
> Cc: wenzhuo...@intel.com; konstantin.anan...@intel.com;
> qi.z.zh...@intel.com; David Harton (dharton)
> Subject: [PATCH v2] net/ixgbe: Restore vlan filter/extend for ixgbevf
>
> ixgbevf vlan strip and extend capabilities were removed when migrating to
> the bit flags implementation.
>
> Restoring the capbility to enable these offloads at configuration time.
>
> Fixes: ec3b1124d14d (\"net/ixgbe: convert to new Rx offloads API\")
> Signed-off-by: David Harton
> ---
>
> v2: removed unused function ixgbe_is_vf()
>
> drivers/net/ixgbe/ixgbe_rxtx.c | 23 ++-
> 1 file changed, 2 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> b/drivers/net/ixgbe/ixgbe_rxtx.c index e92a70fb3..b1b83613e 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2813,23 +2813,6 @@ ixgbe_reset_rx_queue(struct ixgbe_adapter *adapter,
> struct ixgbe_rx_queue *rxq) #endif }
>
> -static int
> -ixgbe_is_vf(struct rte_eth_dev *dev)
> -{
> - struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> -
> - switch (hw->mac.type) {
> - case ixgbe_mac_82599_vf:
> - case ixgbe_mac_X540_vf:
> - case ixgbe_mac_X550_vf:
> - case ixgbe_mac_X550EM_x_vf:
> - case ixgbe_mac_X550EM_a_vf:
> - return 1;
> - default:
> - return 0;
> - }
> -}
> -
> uint64_t
> ixgbe_get_rx_queue_offloads(struct rte_eth_dev *dev) { @@ -2853,15
> +2836,13 @@ ixgbe_get_rx_port_offloads(struct rte_eth_dev *dev)
> DEV_RX_OFFLOAD_TCP_CKSUM |
> DEV_RX_OFFLOAD_KEEP_CRC|
> DEV_RX_OFFLOAD_JUMBO_FRAME |
> +DEV_RX_OFFLOAD_VLAN_FILTER |
> +DEV_RX_OFFLOAD_VLAN_EXTEND |
> DEV_RX_OFFLOAD_SCATTER;
>
> if (hw->mac.type == ixgbe_mac_82598EB)
> offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
>
> - if (ixgbe_is_vf(dev) == 0)
> - offloads |= (DEV_RX_OFFLOAD_VLAN_FILTER |
> - DEV_RX_OFFLOAD_VLAN_EXTEND);
> -
> /*
>* RSC is only supported by 82599 and x540 PF devices in a non-SR-
> IOV
>* mode.
> --
> 2.19.1
Re: [dpdk-dev] [PATCH] net/ixgbe: Restore vlan filter/extend for ixgbvef
Hi
> -Original Message-
> From: Zhao1, Wei
> Sent: Friday, March 08, 2019 1:35 AM
> To: David Harton (dharton) ; dev@dpdk.org; Lu, Wenzhuo
> ; Ananyev, Konstantin
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: Restore vlan filter/extend for
> ixgbvef
>
> Hi, David Harton
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of David Harton
> > Sent: Wednesday, March 6, 2019 11:24 PM
> > To: dev@dpdk.org; Lu, Wenzhuo ; Ananyev,
> > Konstantin
> > Cc: David Harton
> > Subject: [dpdk-dev] [PATCH] net/ixgbe: Restore vlan filter/extend for
> > ixgbvef
> >
> > ixgevf vlan strip and extend capabilities were removed when migrating
> > to the bit flags implementation.
> >
> > Restoring the capbility to enable these offloads at configuration time.
> >
> > Fixes: ec3b1124d14d (\"net/ixgbe: convert to new Rx offloads API\")
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ixgbe/ixgbe_rxtx.c | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index e92a70fb3..95c32257c 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -2853,15 +2853,13 @@ ixgbe_get_rx_port_offloads(struct rte_eth_dev
> > *dev)
> >DEV_RX_OFFLOAD_TCP_CKSUM |
> >DEV_RX_OFFLOAD_KEEP_CRC|
> >DEV_RX_OFFLOAD_JUMBO_FRAME |
> > + DEV_RX_OFFLOAD_VLAN_FILTER |
> > + DEV_RX_OFFLOAD_VLAN_EXTEND |
> >DEV_RX_OFFLOAD_SCATTER;
> >
>
> Why do we set DEV_RX_OFFLOAD_VLAN_EXTEND for ixgbevf ?
> It seems Ixgbevf do not support this configuration. Only pf does.
> But this ixgbe_get_rx_port_offloads () is used by both vf and pf.
I'm personally happy only to move FILTER but I noticed that EXTEND used to be
'supported' and I see references to it elsewhere in ixgbevf code:
ixgbevf_dev_start()
/* Set HW strip */
mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
ETH_VLAN_EXTEND_MASK;
ixgbe_vlan_offload_config()
if (mask & ETH_VLAN_EXTEND_MASK) {
if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
ixgbe_vlan_hw_extend_enable(dev);
else
ixgbe_vlan_hw_extend_disable(dev);
}
These uses imply that EXTEND should be supported, correct?
Thanks,
Dave
>
>
> > if (hw->mac.type == ixgbe_mac_82598EB)
> > offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> >
> > - if (ixgbe_is_vf(dev) == 0)
> > - offloads |= (DEV_RX_OFFLOAD_VLAN_FILTER |
> > -DEV_RX_OFFLOAD_VLAN_EXTEND);
> > -
> > /*
> > * RSC is only supported by 82599 and x540 PF devices in a non-SR-
> > IOV
> > * mode.
> > --
> > 2.19.1
Re: [dpdk-dev] [PATCH] net/ixgbe: Restore vlan filter/extend for ixgbvef
Hi, > -Original Message- > From: Zhang, Qi Z > Sent: Thursday, March 07, 2019 8:11 AM > To: David Harton (dharton) ; dev@dpdk.org; Lu, Wenzhuo > ; Ananyev, Konstantin > Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: Restore vlan filter/extend for > ixgbvef > > Hi > > > -Original Message- > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of David Harton > > Sent: Wednesday, March 6, 2019 11:24 PM > > To: dev@dpdk.org; Lu, Wenzhuo ; Ananyev, > > Konstantin > > Cc: David Harton > > Subject: [dpdk-dev] [PATCH] net/ixgbe: Restore vlan filter/extend for > > ixgbvef > > > > ixgevf vlan strip and extend capabilities were removed when migrating > > to the bit flags implementation. > > > > Restoring the capbility to enable these offloads at configuration time. > > > > Fixes: ec3b1124d14d (\"net/ixgbe: convert to new Rx offloads API\") > > Signed-off-by: David Harton > > --- > > drivers/net/ixgbe/ixgbe_rxtx.c | 6 ++ > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c > > b/drivers/net/ixgbe/ixgbe_rxtx.c index e92a70fb3..95c32257c 100644 > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c > > @@ -2853,15 +2853,13 @@ ixgbe_get_rx_port_offloads(struct rte_eth_dev > > *dev) > >DEV_RX_OFFLOAD_TCP_CKSUM | > >DEV_RX_OFFLOAD_KEEP_CRC| > >DEV_RX_OFFLOAD_JUMBO_FRAME | > > + DEV_RX_OFFLOAD_VLAN_FILTER | > > + DEV_RX_OFFLOAD_VLAN_EXTEND | > >DEV_RX_OFFLOAD_SCATTER; > > > > if (hw->mac.type == ixgbe_mac_82598EB) > > offloads |= DEV_RX_OFFLOAD_VLAN_STRIP; > > > > - if (ixgbe_is_vf(dev) == 0) > > Patchwork shows "build patch error" for ci/Performance-Testing. > Seems this is the only place ixgbe_is_vf is used, so it can also be > removed Yes, I didn't notice that. Assuming people are good with the fundamental change I'll post a new patch with ixgbe_is_vf removed. Thanks, Dave > > > - offloads |= (DEV_RX_OFFLOAD_VLAN_FILTER | > > -DEV_RX_OFFLOAD_VLAN_EXTEND); > > - > > /* > > * RSC is only supported by 82599 and x540 PF devices in a non-SR- > IOV > > * mode. > > -- > > 2.19.1
Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory
> -Original Message-
> From: Wiles, Keith
> Sent: Friday, December 07, 2018 6:41 PM
> To: David Harton (dharton)
> Cc: dev@dpdk.org; Burakov, Anatoly
> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory
>
>
>
> > On Dec 7, 2018, at 3:24 PM, David Harton wrote:
> >
> > The zalloc and calloc functions do not actually zero the memory.
> > Added memset to rte_zmalloc_socket() so allocated memory is cleared.
> >
> > Signed-off-by: David Harton
> > ---
> > lib/librte_eal/common/rte_malloc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/common/rte_malloc.c
> > b/lib/librte_eal/common/rte_malloc.c
> > index 0da5ad5e8..be382e534 100644
> > --- a/lib/librte_eal/common/rte_malloc.c
> > +++ b/lib/librte_eal/common/rte_malloc.c
> > @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned
> > align) void * rte_zmalloc_socket(const char *type, size_t size,
> > unsigned align, int socket) {
> > - return rte_malloc_socket(type, size, align, socket);
> > + void *new_ptr = rte_malloc_socket(type, size, align, socket);
> > + if (new_ptr) memset(new_ptr, 0, size);
>
> Someone will hate me, but the memset() line should be on the next line not
> on the ‘if’ line. It does not explicitly state in the coding style, but do
> not see any example in the coding style on having the one line statement
> on the line of the ‘if’.
>
> What is the ruling here, I would suggest it be on the next line?
FWIW, I copied the pattern from rte_free() but I it is the only use in the file.
I have no problems changing it and fixing rte_free() too if that is the desire.
>
> > + return new_ptr;
> > }
> >
> > /*
> > --
> > 2.19.1
> >
>
> Regards,
> Keith
Re: [dpdk-dev] vmxnet3 Maintainer?
Excellent! I unicasted Yong earlier when I saw the ACKs to vmxnet3 patches. I'll add you there. Thanks Louis, Dave > -Original Message- > From: Louis Luo > Sent: Wednesday, May 02, 2018 5:55 PM > To: Thomas Monjalon ; David Harton (dharton) > > Cc: dev@dpdk.org; Yong Wang > Subject: Re: [dpdk-dev] vmxnet3 Maintainer? > > Hi Dave, > > The previous maintainer, Shri, has left VMware. Now Yong is taking the > maintainer's role and I will work on the vmxnet3 driver. I haven't heard > from Shri about the issue before. We can setup a testbed to reproduce the > issue. Can you describe your setup? The issue reported by Paul in the > following link was from 2016 and the ESX version was pretty old. Which > ESX/Linux/DPDK version do you use? > > Thanks, > Louis > > On 5/2/18, 2:02 PM, "dev on behalf of Thomas Monjalon" boun...@dpdk.org on behalf of tho...@monjalon.net> wrote: > > 02/05/2018 22:38, David Harton (dharton): > > I've encountered a couple scenarios with vmxnet3 (dpdk 2.2 and > 16.07) that look pretty much identical to this: > > https://urldefense.proofpoint.com/v2/url?u=http- > 3A__dpdk.org_ml_archives_users_2016- > 2DOctober_001063.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=y25HyFjCYzilk6V6 > ZP1fv_ZqnUERYZnHOMXviGNhdyQ&m=b0fOlYK3AC8HKUkrELsQZXwDfRGxc44mixhpABK56JM& > s=pcQXE-BzpV_QVQbRgScrJzo9wpcFcmAYpJ_xN10qOmM&e= > > > > Do we have a maintainer for the vmxnet3 driver? I tried emailing > the one listing in the MAINTAINERS file but the mail bounced. > > The maintainer changed recently. > It is Yong Wang (Cc'ed). > > > Is anyone familiar with the issue described in the thread above? > > > > > >
[dpdk-dev] vmxnet3 Maintainer?
I've encountered a couple scenarios with vmxnet3 (dpdk 2.2 and 16.07) that look pretty much identical to this: http://dpdk.org/ml/archives/users/2016-October/001063.html Do we have a maintainer for the vmxnet3 driver? I tried emailing the one listing in the MAINTAINERS file but the mail bounced. Is anyone familiar with the issue described in the thread above? Thanks, Dave
Re: [dpdk-dev] Retire x86 32 bit?
It is used and tested in production and non-production environments. Regards, Dave > -Original Message- > From: dev On Behalf Of Stephen Hemminger > Sent: Tuesday, April 17, 2018 2:31 PM > To: dev@dpdk.org > Subject: [dpdk-dev] Retire x86 32 bit? > > I wonder if x86 32 bit is still useful? > Many distributions no longer support it, and not sure if it is tested > througly by anyone. > > Maybe time to deprecate it (gradually)?
Re: [dpdk-dev] [PATCH] net/i40e: fix VF reset stats crash
> -Original Message- > From: Van Haaren, Harry [mailto:harry.van.haa...@intel.com] > > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of David C Harton > > > > Calling i40evf_dev_xstats_reset can sometimes crash. > > > > Fixed issue by adding a pstats NULL check. > > Perhaps add a note here, that the root cause of (pstats == NULL) is a > valid error return from i40evf_query_stats(), if the vf command fails to > execute. > > Hence, we are not hiding symptoms of a different issue here, this NULL > check is required to validate the returned values. > NULL is returned if the call fails. If desired I can capture the return code and base the check off the rc instead. That's the pattern used in a couple other places. Thanks, Dave
Re: [dpdk-dev] [PATCH v4] ethdev: allow returning error on VLAN offload configuration
> -Original Message- > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] > > On 9/1/2017 3:36 AM, David Harton wrote: > > Some devices may not support or fail setting VLAN offload > > configuration based on dynamic circurmstances so the > > vlan_offload_set_t vector is modified to return an int so the caller > > can determine success or not. > > > > rte_eth_dev_set_vlan_offload is updated to return the value provided > > by the vector when called along with restoring the original offload > > configs on failure. > > > > Existing vlan_offload_set_t vectors are modified to return an int. > > Majority of cases return 0 but a few that actually can fail now return > > their failure codes. > > > > Finally, a vlan_offload_set_t vector is added to virtio to facilitate > > dynamically turning VLAN strip on or off. > > > > Signed-off-by: David Harton > > Hi David, > > This patch conflicts against latest version, would you mind rebasing it on > top of latest next-net? Sure Ferruh, no problem. Thanks, Dave > > Thanks, > ferruh
Re: [dpdk-dev] [PATCH v5] ethdev: add return code to rte_eth_stats_reset()
> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
>
> On 9/1/2017 3:26 AM, David Harton wrote:
> > Some devices do not support reset of eth stats. An application may
> > need to know not to clear shadow stats if the device cannot.
> >
> > rte_eth_stats_reset is updated to provide a return code to share
> > whether the device supports reset or not.
> >
> > Signed-off-by: David Harton
> > ---
> >
> > v5:
> > * squashed doc patch
> > * moved rel_note change from ABI to API section
> >
> > v4:
> > * commented return values
> >
> > v3:
> > * overcame noob errors and figured out patch challenged
> >
> > v2:
> > * fixed soft tab issue inserted while moving changes
> >
> > doc/guides/rel_notes/release_17_11.rst | 13 +
> > lib/librte_ether/rte_ethdev.c | 8 +---
> > lib/librte_ether/rte_ethdev.h | 6 +-
> > 3 files changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_17_11.rst
> > b/doc/guides/rel_notes/release_17_11.rst
> > index 22df4fd..6282667 100644
> > --- a/doc/guides/rel_notes/release_17_11.rst
> > +++ b/doc/guides/rel_notes/release_17_11.rst
> > @@ -110,6 +110,19 @@ API Changes
> > Also, make sure to start the actual text at the margin.
> > =
> >
> > +* **Modified the return type of rte_eth_stats_reset.**
> > +
> > + Changed return type of ``rte_eth_stats_reset`` from ``void`` to
> > + ``int`` so the caller may know whether a device supports the
> > + operation or not and if the operation was carried out.
> > +
>
> > +* **Modified the vlan_offload_set_t function prototype in the ethdev
> > +library.**
> > +
> > + Changed the function prototype of ``vlan_offload_set_t``. The
> > + return value has been changed from ``void`` to ``int`` so the
> > + caller to knows whether the backing device supports the operation
> > + or if the operation was successfully performed.
> > +
>
> Is this addition to the document related to this patch?
Good catch. No. :(
I must have mishandled the rebase I did to update this patch. V6 coming.
Would be great if you could re-ACK afterwards so this one can move.
Thanks,
Dave
>
> > * **Modified the vlan_offload_set_t function prototype in the ethdev
> > library.**
> >
> >Changed the function prototype of ``vlan_offload_set_t``. The
> > return value diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 05e52b8..f0f1775 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1341,17 +1341,19 @@ struct rte_eth_dev *
> > return 0;
> > }
> >
> > -void
> > +int
> > rte_eth_stats_reset(uint8_t port_id)
> > {
> > struct rte_eth_dev *dev;
> >
> > - RTE_ETH_VALID_PORTID_OR_RET(port_id);
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > dev = &rte_eth_devices[port_id];
> >
> > - RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset);
> > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_reset, -ENOTSUP);
> > (*dev->dev_ops->stats_reset)(dev);
> > dev->data->rx_mbuf_alloc_failed = 0;
> > +
> > + return 0;
> > }
> >
> > static int
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index 7254fd0..9110725 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2246,8 +2246,12 @@ int rte_eth_tx_queue_setup(uint8_t port_id,
> uint16_t tx_queue_id,
> > *
> > * @param port_id
> > * The port identifier of the Ethernet device.
> > + * @return
> > + * - (0) if device notified to reset stats.
> > + * - (-ENOTSUP) if hardware doesn't support.
> > + * - (-ENODEV) if *port_id* invalid.
> > */
> > -void rte_eth_stats_reset(uint8_t port_id);
> > +int rte_eth_stats_reset(uint8_t port_id);
> >
> > /**
> > * Retrieve names of extended statistics of an Ethernet device.
> >
Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist symbols
> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Thursday, September 14, 2017 6:10 AM
> To: David Harton (dharton) ;
> konstantin.anan...@intel.com
> Cc: dev@dpdk.org; Wenzhuo Lu
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist
> symbols
>
> On 8/29/2017 5:23 PM, Ferruh Yigit wrote:
> > On 8/28/2017 6:13 PM, David Harton (dharton) wrote:
> >>
> >>
> >>> -Original Message-
> >>> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> >>> Sent: Monday, August 28, 2017 9:27 AM
> >>> To: David Harton (dharton) ;
> >>> konstantin.anan...@intel.com
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate
> >>> filterlist symbols
> >>>
> >>> On 8/25/2017 8:21 PM, David Harton wrote:
> >>>> Some compilers generate warnings for duplicate symbols for the set
> >>>> of filter lists current defined in ixgbe_ethdev.h.
> >>>> This commits moves the defintion and declaration to the source file
> >>>> that actually uses them and provides a function to initialize the
> >>>> values akin to its flush function.
> >>>>
> >>>> Signed-off-by: David Harton
> >>>> ---
> >>>> drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++--
> >>>> drivers/net/ixgbe/ixgbe_ethdev.h | 7 +--
> >>>> drivers/net/ixgbe/ixgbe_flow.c | 18 ++
> >>>> 3 files changed, 21 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> index 1ec5aaf..ed21af5 100644
> >>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> @@ -1332,12 +1332,8 @@ struct rte_ixgbe_xstats_name_off {
> >>>> /* initialize l2 tunnel filter list & hash */
> >>>> ixgbe_l2_tn_filter_init(eth_dev);
> >>>>
> >>>> -TAILQ_INIT(&filter_ntuple_list);
> >>>> -TAILQ_INIT(&filter_ethertype_list);
> >>>> -TAILQ_INIT(&filter_syn_list);
> >>>> -TAILQ_INIT(&filter_fdir_list);
> >>>> -TAILQ_INIT(&filter_l2_tunnel_list);
> >>>> -TAILQ_INIT(&ixgbe_flow_list);
> >>>> +/* initialize flow filter lists */
> >>>> +ixgbe_filterlist_init();
> >>>
> >>> Will it work if list initialization converted to the static
> >>> initialization? That would remove requirement of this function call.
> >>
> >> I was trying to keep the behavior the same:
> >> .probe -> eth_ixgbe_dev_init -> ixgbe_filterlist_init
> >> .remove -> eth_ixgbe_dev_uninit -> ixgbe_filterlist_flush
> >>
> >> While I think the below would work I always like mirror/parallel
> relationships like ctor/dtor for objects.
> >
> > I would agree if ixgbe_filterlist_init() initializes something more
> > than list itself, but for this case initialization can be avoided
> > completely which I would prefer.
> >
> > Driver maintainer also cc'ed for more comment.
>
> It seems there is no more comment. So, I suggest lets focus on your fix,
> my suggestion to convert to static initialization can be done later if
> desired.
>
> Still, what do you think about moving declaration of structs as well to
> the ixgbe_flow.c? After your change, only ixgbe_flow.c will be using them,
> no need to keep them in ixgbe_ethdev.h.
Works for me. Let me make the changes and test.
Regards,
Dave
>
> Thanks,
> ferruh
>
> >
> >>
> >> Is that reasonable?
> >>
> >> Thanks,
> >> Dave
> >>
> >>>
> >>> something like:
> >>>
> >>> diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> >>> b/drivers/net/ixgbe/ixgbe_flow.c index c8645f056..6184eb4af 100644
> >>> --- a/drivers/net/ixgbe/ixgbe_flow.c
> >>> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> >>> @@ -79,6 +79,24 @@
> >>> #define IXGBE_MAX_N_TUPLE_PRIO 7
> >>> #define IXGBE_MAX_FLX_SOURCE_OFF 62
> >>>
> >>> +static TAILQ_HEAD(ixgbe_ntuple_filter_list, ixgbe_ntuple_filter_ele)
> >>> + filter_ntuple_list =
> >>> +TAILQ_HEAD_INITIALIZER(filter_ntuple_list);
> >>> +
> >>> +static TAILQ_HEAD(ixgbe_ethertype_filter_list,
> >>> ixgbe_ethertype_filter_ele)
> >>> + filter_ethertype_list =
> >>> TAILQ_HEAD_INITIALIZER(filter_ethertype_list);
> >>> +
> >>> +static TAILQ_HEAD(ixgbe_syn_filter_list, ixgbe_eth_syn_filter_ele)
> >>> + filter_syn_list = TAILQ_HEAD_INITIALIZER(filter_syn_list);
> >>> +
> >>> +static TAILQ_HEAD(ixgbe_fdir_rule_filter_list, ixgbe_fdir_rule_ele)
> >>> + filter_fdir_list = TAILQ_HEAD_INITIALIZER(filter_fdir_list);
> >>> +
> >>> +static TAILQ_HEAD(ixgbe_l2_tunnel_filter_list,
> >>> ixgbe_eth_l2_tunnel_conf_ele)
> >>> + filter_l2_tunnel_list =
> >>> TAILQ_HEAD_INITIALIZER(filter_l2_tunnel_list);
> >>> +
> >>> +static TAILQ_HEAD(ixgbe_flow_mem_list, ixgbe_flow_mem)
> >>> + ixgbe_flow_list = TAILQ_HEAD_INITIALIZER(ixgbe_flow_list);
> >>> +
> >
Re: [dpdk-dev] [PATCH v2] i40e: fix i40evf_add_mac_addr to permit multicast addresses
Hi Beilei,
> -Original Message-
> From: Xing, Beilei [mailto:beilei.x...@intel.com]
>
> Hi Harton,
>
> > -Original Message-
> > From: David Harton [mailto:dharton@cpp-rtpbld-31.cpprtplab]
> >
> > From: David Harton
> >
> > The i40e maintains a single MAC filter table for both unicast and
> > multicast addresses. The i40e_validate_mac_addr function was
> > preventing multicast addresses from being added to the table via
> > i40evf_add_mac_addr. Fixed the issue by adjusting the check in
> i40evf_add_mac_addr.
> >
> > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> > Fixes: 97ac72aa71a9 ("i40e: support setting VF MAC address")
> >
> > Signed-off-by: David Harton
> > ---
> >
> > v2
> > * Removed multicast check in i40evf_add_mac_addr.
> >
> > v1
> > * Removed multicast check in i40e_validate_mac_addr.
> >
> > drivers/net/i40e/i40e_ethdev_vf.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index f6d8293..5916d11 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -888,7 +888,7 @@ i40evf_add_mac_addr(struct rte_eth_dev *dev,
> > int err;
> > struct vf_cmd_info args;
> >
> > - if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
> > + if (is_zero_ether_addr(addr) != I40E_SUCCESS) {
>
> Thanks for the patch, there's some mistake with my last comment.
Actually, I think the logic above works but I do agree it is confusing
but I wanted to use your suggestion to speed up the process. :)
> * @return
> * True (1) if the given ethernet address is filled with zeros;
> * false (0) otherwise.
> */
> According to the comment of is_zero_ether_addr function, return value
> should be 0 or 1.
> So you should use if (!is_zero_ether_addr(addr)) here.
I don't think so. I think the logic that should be used is
if (is_zero_ether_addr(addr)) {
because we want to report an error if the addr is all 0's.
Please let me know if you agree and I'll post this patch instead.
Thanks,
Dave
>
> > PMD_DRV_LOG(ERR, "Invalid mac:%x:%x:%x:%x:%x:%x",
> > addr->addr_bytes[0], addr->addr_bytes[1],
> > addr->addr_bytes[2], addr->addr_bytes[3],
> > --
> > 2.10.3.dirty
Re: [dpdk-dev] [PATCH] i40e: fix i40e_validate_mac_addr to permit multicast addresses
Hi Beilei,
> -Original Message-
> From: Xing, Beilei [mailto:beilei.x...@intel.com]
>
> Hi,
>
> > -Original Message-
> > From: David Harton [mailto:dhar...@cisco.com]
> >
> > The i40e maintains a single MAC filter table for both unicast and
> > multicast addresses. The i40e_validate_mac_addr function was
> > preventing multicast addresses from being added to the table via
> > i40evf_add_mac_addr. Fixed the issue by removing the multicast
> > address check in i40e_validate_mac_addr.
> >
>
> Maybe fix line is needed here.
Please elaborate. If feel this is a regression and you want
me to add a fixline can you offer the offending commit?
>
> > Signed-off-by: David Harton
> > ---
> > drivers/net/i40e/base/i40e_common.c | 12 +---
> > drivers/net/i40e/i40e_ethdev.c | 3 ++-
> > 2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/i40e/base/i40e_common.c
> > b/drivers/net/i40e/base/i40e_common.c
> > index 900d379..9779854 100644
> > --- a/drivers/net/i40e/base/i40e_common.c
> > +++ b/drivers/net/i40e/base/i40e_common.c
> > @@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded i40e_ptype_lookup[]
> > = {
> >
> >
> > /**
> > - * i40e_validate_mac_addr - Validate unicast MAC address
> > + * i40e_validate_mac_addr - Validate MAC address
> > * @mac_addr: pointer to MAC address
> > *
> > - * Tests a MAC address to ensure it is a valid Individual Address
> > + * Tests a MAC address to ensure it is a valid Address
> > **/
> > enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr) { @@ -
> > 980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8
> > *mac_addr)
> >
> > DEBUGFUNC("i40e_validate_mac_addr");
> >
> > - /* Broadcast addresses ARE multicast addresses
> > -* Make sure it is not a multicast address
> > + /*
> > * Reject the zero address
> > */
> > - if (I40E_IS_MULTICAST(mac_addr) ||
> > - (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
> > - mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
> > + if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
> > + mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
> > status = I40E_ERR_INVALID_MAC_ADDR;
> >
>
> We'd better not to change the base code which is from kernel driver. How
> about changing the i40evf_add_mac_addr function as following:
> - if (i40e_validate_mac_addr(addr->addr_bytes) != I40E_SUCCESS) {
> + if (is_zero_ether_addr (addr->addr_bytes) != I40E_SUCCESS) {
>
> Then following modification in eth_i40e_dev_init function can be removed.
> But we should also pay attention to other functions which call
> i40evf_add_mac_addr.
Sure. I'll post a new patch as suggested once the fixline question is
answered.
I was trying to fix the issue the way I did in case i40e passthru
wanted to benefit. Your suggestion will work for the VF as the only
other caller for i40evf_add_mac_addr() performs its own macaddr validation.
Thanks,
Dave
>
> Beilei
>
> > return status;
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
> >
> > /* Get and check the mac address */
> > i40e_get_mac_addr(hw, hw->mac.addr);
> > - if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
> > + if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
> > + I40E_IS_MULTICAST(hw->mac.addr)) {
> > PMD_INIT_LOG(ERR, "mac address is not valid");
> > ret = -EIO;
> > goto err_get_mac_addr;
> > --
> > 2.10.3.dirty
Re: [dpdk-dev] [PATCH] i40e: fix i40e_validate_mac_addr to permit multicast addresses
Hi Jingjing/Beilei,
A kind reminder to review the patch and the discussion between
Ferruh and myself.
Thanks,
Dave
> -Original Message-
> From: David Harton (dharton)
> > -Original Message-
> > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> >
> > On 8/22/2017 11:21 PM, David Harton wrote:
> > > The i40e maintains a single MAC filter table for both unicast and
> > > multicast addresses. The i40e_validate_mac_addr function was
> > > preventing multicast addresses from being added to the table via
> > > i40evf_add_mac_addr. Fixed the issue by removing the multicast
> > > address check in i40e_validate_mac_addr.
> > >
> > > Signed-off-by: David Harton
> > > ---
> > > drivers/net/i40e/base/i40e_common.c | 12 +---
> > > drivers/net/i40e/i40e_ethdev.c | 3 ++-
> > > 2 files changed, 7 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/base/i40e_common.c
> > > b/drivers/net/i40e/base/i40e_common.c
> > > index 900d379..9779854 100644
> > > --- a/drivers/net/i40e/base/i40e_common.c
> > > +++ b/drivers/net/i40e/base/i40e_common.c
> > > @@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded
> > > i40e_ptype_lookup[] = {
> > >
> > >
> > > /**
> > > - * i40e_validate_mac_addr - Validate unicast MAC address
> > > + * i40e_validate_mac_addr - Validate MAC address
> > > * @mac_addr: pointer to MAC address
> > > *
> > > - * Tests a MAC address to ensure it is a valid Individual Address
> > > + * Tests a MAC address to ensure it is a valid Address
> > > **/
> > > enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr) { @@
> > > -980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8
> > > *mac_addr)
> > >
> > > DEBUGFUNC("i40e_validate_mac_addr");
> > >
> > > - /* Broadcast addresses ARE multicast addresses
> > > - * Make sure it is not a multicast address
> > > + /*
> > >* Reject the zero address
> > >*/
> > > - if (I40E_IS_MULTICAST(mac_addr) ||
> > > - (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
> > > - mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
> > > + if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
> > > + mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
> > > status = I40E_ERR_INVALID_MAC_ADDR;
> > >
> > > return status;
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
> > >
> > > /* Get and check the mac address */
> > > i40e_get_mac_addr(hw, hw->mac.addr);
> > > - if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
> > > + if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
> > > + I40E_IS_MULTICAST(hw->mac.addr)) {
> >
> > As far as I can see this is to set PF permanent mac address during
> > init(),
> > i40e_macaddr_add() can be used to set multicast address filters.
> > Why do you want to set multicast address as mac permanent address?
> >
> > Any chance that you want to update i40evf_add_mac_addr() to let
> > multicast addresses?
>
> No. I'm preserving the existing behavior here. The previous call used to
> ensure that the permanent mac address here was both non-null and not a
> multi-cast address.
>
> Since I removed the multi cast check from i40e_validate_mac_addr() I added
> the multicast check here. If "not valid" or a multicast address fail.
>
> Regards,
> Dave
>
> >
> > > PMD_INIT_LOG(ERR, "mac address is not valid");
> > > ret = -EIO;
> > > goto err_get_mac_addr;
> > >
Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist symbols
Hi Konstantin,
A gentle reminder to weigh in on this patch and the
discussion between Ferruh and myself below.
Thanks,
Dave
> -Original Message-
> From: David Harton (dharton)
>
> > -Original Message-
> > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> >
> > On 8/25/2017 8:21 PM, David Harton wrote:
> > > Some compilers generate warnings for duplicate symbols for the set
> > > of filter lists current defined in ixgbe_ethdev.h.
> > > This commits moves the defintion and declaration to the source file
> > > that actually uses them and provides a function to initialize the
> > > values akin to its flush function.
> > >
> > > Signed-off-by: David Harton
> > > ---
> > > drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++--
> > > drivers/net/ixgbe/ixgbe_ethdev.h | 7 +--
> > > drivers/net/ixgbe/ixgbe_flow.c | 18 ++
> > > 3 files changed, 21 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 1ec5aaf..ed21af5 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -1332,12 +1332,8 @@ struct rte_ixgbe_xstats_name_off {
> > > /* initialize l2 tunnel filter list & hash */
> > > ixgbe_l2_tn_filter_init(eth_dev);
> > >
> > > - TAILQ_INIT(&filter_ntuple_list);
> > > - TAILQ_INIT(&filter_ethertype_list);
> > > - TAILQ_INIT(&filter_syn_list);
> > > - TAILQ_INIT(&filter_fdir_list);
> > > - TAILQ_INIT(&filter_l2_tunnel_list);
> > > - TAILQ_INIT(&ixgbe_flow_list);
> > > + /* initialize flow filter lists */
> > > + ixgbe_filterlist_init();
> >
> > Will it work if list initialization converted to the static
> > initialization? That would remove requirement of this function call.
>
> I was trying to keep the behavior the same:
> .probe -> eth_ixgbe_dev_init -> ixgbe_filterlist_init
> .remove -> eth_ixgbe_dev_uninit -> ixgbe_filterlist_flush
>
> While I think the below would work I always like mirror/parallel
> relationships like ctor/dtor for objects.
>
> Is that reasonable?
>
> Thanks,
> Dave
>
> >
> > something like:
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> > b/drivers/net/ixgbe/ixgbe_flow.c index c8645f056..6184eb4af 100644
> > --- a/drivers/net/ixgbe/ixgbe_flow.c
> > +++ b/drivers/net/ixgbe/ixgbe_flow.c
> > @@ -79,6 +79,24 @@
> > #define IXGBE_MAX_N_TUPLE_PRIO 7
> > #define IXGBE_MAX_FLX_SOURCE_OFF 62
> >
> > +static TAILQ_HEAD(ixgbe_ntuple_filter_list, ixgbe_ntuple_filter_ele)
> > + filter_ntuple_list =
> > +TAILQ_HEAD_INITIALIZER(filter_ntuple_list);
> > +
> > +static TAILQ_HEAD(ixgbe_ethertype_filter_list,
> > ixgbe_ethertype_filter_ele)
> > + filter_ethertype_list =
> > TAILQ_HEAD_INITIALIZER(filter_ethertype_list);
> > +
> > +static TAILQ_HEAD(ixgbe_syn_filter_list, ixgbe_eth_syn_filter_ele)
> > + filter_syn_list = TAILQ_HEAD_INITIALIZER(filter_syn_list);
> > +
> > +static TAILQ_HEAD(ixgbe_fdir_rule_filter_list, ixgbe_fdir_rule_ele)
> > + filter_fdir_list = TAILQ_HEAD_INITIALIZER(filter_fdir_list);
> > +
> > +static TAILQ_HEAD(ixgbe_l2_tunnel_filter_list,
> > ixgbe_eth_l2_tunnel_conf_ele)
> > + filter_l2_tunnel_list =
> > TAILQ_HEAD_INITIALIZER(filter_l2_tunnel_list);
> > +
> > +static TAILQ_HEAD(ixgbe_flow_mem_list, ixgbe_flow_mem)
> > + ixgbe_flow_list = TAILQ_HEAD_INITIALIZER(ixgbe_flow_list);
> > +
Re: [dpdk-dev] [PATCH v4] ethdev: allow returning error on VLAN offload configuration
> -Original Message-
> From: Hemant Agrawal [mailto:hemant.agra...@nxp.com]
>
> On 9/1/2017 6:24 PM, David Harton (dharton) wrote:
> >
> >> -Original Message-
> >> From: Hemant Agrawal [mailto:hemant.agra...@nxp.com]
> >>
> >> On 9/1/2017 8:06 AM, David Harton wrote:
> >>> Some devices may not support or fail setting VLAN offload
> >>> configuration based on dynamic circurmstances so the
> >>> vlan_offload_set_t vector is modified to return an int so the caller
> >>> can determine success or not.
> >>>
> >>> rte_eth_dev_set_vlan_offload is updated to return the value provided
> >>> by the vector when called along with restoring the original offload
> >>> configs on failure.
> >>>
> >>> Existing vlan_offload_set_t vectors are modified to return an int.
> >>> Majority of cases return 0 but a few that actually can fail now
> >>> return their failure codes.
> >>>
> >>> Finally, a vlan_offload_set_t vector is added to virtio to
> >>> facilitate dynamically turning VLAN strip on or off.
> >>>
> >>> Signed-off-by: David Harton
> >>> ---
>
>
>
> >>> diff --git a/lib/librte_ether/rte_ethdev.c
> >> b/lib/librte_ether/rte_ethdev.c
> >>> index 0597641..05e52b8 100644
> >>> --- a/lib/librte_ether/rte_ethdev.c
> >>> +++ b/lib/librte_ether/rte_ethdev.c
> >>> @@ -2048,29 +2048,35 @@ struct rte_eth_dev *
> >>> struct rte_eth_dev *dev;
> >>> int ret = 0;
> >>> int mask = 0;
> >>> - int cur, org = 0;
> >>> + int cur, orig = 0;
> >>> + uint8_t orig_strip, orig_filter, orig_extend;
> >>>
> >>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >>> dev = &rte_eth_devices[port_id];
> >>>
> >>> + /* save original values in case of failure */
> >>> + orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> >>> + orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> >>> + orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
> >>> +
> >>> /*check which option changed by application*/
> >>> cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
> >>> - org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> >>> - if (cur != org) {
> >>> + orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> >>> + if (cur != orig) {
> >>> dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
> >>> mask |= ETH_VLAN_STRIP_MASK;
> >>> }
> >>>
> >>> cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
> >>> - org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> >>> - if (cur != org) {
> >>> + orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> >>> + if (cur != orig) {
> >>> dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
> >>> mask |= ETH_VLAN_FILTER_MASK;
> >>> }
> >>>
> >>> cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
> >>> - org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> >>> - if (cur != org) {
> >>> + orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> >>> + if (cur != orig) {
> >>> dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
> >>> mask |= ETH_VLAN_EXTEND_MASK;
> >>> }
> >>> @@ -2080,7 +2086,13 @@ struct rte_eth_dev *
> >>> return ret;
> >>>
> >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
> >>> - (*dev->dev_ops->vlan_offload_set)(dev, mask);
> >>> + ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> >>> + if (ret) {
> >>> + /* hit an error restore original values */
> >>> + dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
> >>> + dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
> >>> + dev->data->dev_conf.rxmode.hw_vlan_extend = orig_extend;
> >>> + }
> >>>
> >> Currently vlan offload is combining three functions:
> >> strip, filter and extend.
> >>
> >> Not all PMDs in DPDK support all of three.
> >> Should not the error we extended to reflect, w
Re: [dpdk-dev] [PATCH v4] ethdev: allow returning error on VLAN offload configuration
> -Original Message- > From: Hemant Agrawal [mailto:hemant.agra...@nxp.com] > Sent: Friday, September 01, 2017 3:41 AM > To: David Harton (dharton) ; tho...@monjalon.net; > ferruh.yi...@intel.com; ajit.khapa...@broadcom.com; John Daley (johndale) > ; konstantin.anan...@intel.com; jingjing...@intel.com; > beilei.x...@intel.com; jing.d.c...@intel.com; adrien.mazarg...@6wind.com; > nelio.laranje...@6wind.com; alejandro.luc...@netronome.com; > rasesh.m...@cavium.com; harish.pa...@cavium.com; skh...@vmware.com; > y...@fridaylinux.org; maxime.coque...@redhat.com; > allain.leg...@windriver.com > Cc: dev@dpdk.org > Subject: Re: [PATCH v4] ethdev: allow returning error on VLAN offload > configuration > > On 9/1/2017 8:06 AM, David Harton wrote: > > Some devices may not support or fail setting VLAN offload > > configuration based on dynamic circurmstances so the > > vlan_offload_set_t vector is modified to return an int so the caller > > can determine success or not. > > > > rte_eth_dev_set_vlan_offload is updated to return the value provided > > by the vector when called along with restoring the original offload > > configs on failure. > > > > Existing vlan_offload_set_t vectors are modified to return an int. > > Majority of cases return 0 but a few that actually can fail now return > > their failure codes. > > > > Finally, a vlan_offload_set_t vector is added to virtio to facilitate > > dynamically turning VLAN strip on or off. > > > > Signed-off-by: David Harton > > --- > > > > v4 > > * Modified commit message heading > > * Moved rel_note comments from ABI to API section > > * Renamed locals of rte_eth_dev_set_vlan_offload from 'org*' to 'orig*' > > > > v3 > > * Fixed a format error. > > * Apologies...need to figure out why checkpatches.pl keeps saying > > valid patch when I've got soft tabs. > > > > v2 > > * Fixed a missed format error. > > * Removed vlan offload vector call casts and replaced with checks > > for return values. > > > > v1 > > * This is an ABI breakage that has been previously negotiated > > with Thomas and the proposed rel note change is included as well. > > > > doc/guides/rel_notes/release_17_11.rst | 8 +++- > > drivers/net/avp/avp_ethdev.c | 12 +--- > > drivers/net/bnxt/bnxt_ethdev.c | 9 ++--- > > drivers/net/dpaa2/dpaa2_ethdev.c | 13 ++--- > > drivers/net/e1000/em_ethdev.c | 12 +--- > > drivers/net/e1000/igb_ethdev.c | 12 +--- > > drivers/net/enic/enic_ethdev.c | 8 +--- > > drivers/net/fm10k/fm10k_ethdev.c | 3 ++- > > drivers/net/i40e/i40e_ethdev.c | 11 --- > > drivers/net/i40e/i40e_ethdev_vf.c | 9 ++--- > > drivers/net/ixgbe/ixgbe_ethdev.c | 25 ++--- > > drivers/net/mlx5/mlx5.h| 2 +- > > drivers/net/mlx5/mlx5_vlan.c | 3 ++- > > drivers/net/nfp/nfp_net.c | 13 +++-- > > drivers/net/qede/qede_ethdev.c | 9 +++-- > > drivers/net/virtio/virtio_ethdev.c | 21 + > > drivers/net/vmxnet3/vmxnet3_ethdev.c | 10 +++--- > > lib/librte_ether/rte_ethdev.c | 28 --- > - > > lib/librte_ether/rte_ethdev.h | 2 +- > > 19 files changed, 155 insertions(+), 55 deletions(-) > > > > diff --git a/doc/guides/rel_notes/release_17_11.rst > > b/doc/guides/rel_notes/release_17_11.rst > > index 170f4f9..22df4fd 100644 > > --- a/doc/guides/rel_notes/release_17_11.rst > > +++ b/doc/guides/rel_notes/release_17_11.rst > > @@ -110,6 +110,13 @@ API Changes > > Also, make sure to start the actual text at the margin. > > = > > > > +* **Modified the vlan_offload_set_t function prototype in the ethdev > > +library.** > > + > > + Changed the function prototype of ``vlan_offload_set_t``. The > > + return value has been changed from ``void`` to ``int`` so the > > + caller to knows whether the backing device supports the operation > > + or if the operation was successfully performed. > > + > > > > ABI Changes > > --- > > @@ -125,7 +132,6 @@ ABI Changes > > = > > > > > > - > > Shared Library Versions > > --- > > > > diff --git a/drivers/net/avp/avp_ethdev.c > &
Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int
Hi Thomas,
> -Original Message-
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Thursday, August 31, 2017 6:04 PM
> To: David Harton (dharton)
> Cc: dev@dpdk.org; ferruh.yi...@intel.com; ajit.khapa...@broadcom.com; John
> Daley (johndale) ; konstantin.anan...@intel.com;
> jingjing...@intel.com; beilei.x...@intel.com; jing.d.c...@intel.com;
> adrien.mazarg...@6wind.com; nelio.laranje...@6wind.com;
> alejandro.luc...@netronome.com; hemant.agra...@nxp.com;
> rasesh.m...@cavium.com; harish.pa...@cavium.com; skh...@vmware.com;
> y...@fridaylinux.org; maxime.coque...@redhat.com;
> allain.leg...@windriver.com
> Subject: Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to
> return int
>
> Hi,
>
> 25/08/2017 15:47, David Harton:
> > Some devices may not support or fail setting VLAN offload
> > configuration based on dynamic circurmstances so the
> > vlan_offload_set_t vector is modified to return an int so
> > the caller can determine success or not.
>
> I agree with allowing to return an error.
>
> Comments on details below.
>
> The title could be changed to better reflect the purpose:
> ethdev: allow returning error on VLAN configuration
Sure.
>
> > --- a/doc/guides/rel_notes/release_17_11.rst
> > +++ b/doc/guides/rel_notes/release_17_11.rst
> > @@ -124,7 +124,7 @@ ABI Changes
> > Also, make sure to start the actual text at the margin.
> > =
> >
> > -
> > +* Changed return type of ``vlan_offload_set_t`` from ``void`` to ``int``.
>
> It should be referenced as an API change (instead of ABI change).
> (and line spacing must be kept)
Sure I'll move it to API.
Line spacing? You mean 80 char width? I followed an example from a previous
release so I'm not sure what you mean.
>
> We also need to bump the library version but it can be done with
> bigger changes in ethdev.
Ok.
>
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -2049,10 +2049,16 @@ struct rte_eth_dev *
> > int ret = 0;
> > int mask = 0;
> > int cur, org = 0;
> > + uint8_t org_strip, org_filter, org_extend;
> >
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > dev = &rte_eth_devices[port_id];
> >
> > + /* save original values in case of failure */
> > + org_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> > + org_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> > + org_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
>
> Please waste one more char to write "orig" instead of "org".
Will do.
>
> > - (*dev->dev_ops->vlan_offload_set)(dev, mask);
> > + ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> > + if (ret) {
> > + /* hit an error restore original values */
> > + dev->data->dev_conf.rxmode.hw_vlan_strip = org_strip;
> > + dev->data->dev_conf.rxmode.hw_vlan_filter = org_filter;
> > + dev->data->dev_conf.rxmode.hw_vlan_extend = org_extend;
> > + }
>
> Isn't it the responsibility of the PMD to restore values in case of error?
> I understand it is there to factorize error handling code, right?
By setting the dev_conf.rxmode.hw_vlan_* fields this function is claiming
ownership of that data and therefore it should be the one to reset it.
> Do we want to document this behaviour with the ops prototype?
Why? Shouldn't any function that doesn't do what it was asked to do
leave the system in its original state and in fact if it doesn't that
is when it should be documented what the behavior is? Otherwise every
caller has to implement some cleanup code and would have to be given
information to know how to perform that cleanup as well which seems
more complicated.
Thanks,
Dave
Re: [dpdk-dev] [PATCH] i40e: fix i40e_validate_mac_addr to permit multicast addresses
> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Thursday, August 31, 2017 11:54 AM
> To: David Harton (dharton) ; jingjing...@intel.com;
> beilei.x...@intel.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] i40e: fix i40e_validate_mac_addr to permit
> multicast addresses
>
> On 8/22/2017 11:21 PM, David Harton wrote:
> > The i40e maintains a single MAC filter table for both unicast and
> > multicast addresses. The i40e_validate_mac_addr function was
> > preventing multicast addresses from being added to the table via
> > i40evf_add_mac_addr. Fixed the issue by removing the multicast
> > address check in i40e_validate_mac_addr.
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/i40e/base/i40e_common.c | 12 +---
> > drivers/net/i40e/i40e_ethdev.c | 3 ++-
> > 2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/i40e/base/i40e_common.c
> > b/drivers/net/i40e/base/i40e_common.c
> > index 900d379..9779854 100644
> > --- a/drivers/net/i40e/base/i40e_common.c
> > +++ b/drivers/net/i40e/base/i40e_common.c
> > @@ -969,10 +969,10 @@ struct i40e_rx_ptype_decoded i40e_ptype_lookup[]
> > = {
> >
> >
> > /**
> > - * i40e_validate_mac_addr - Validate unicast MAC address
> > + * i40e_validate_mac_addr - Validate MAC address
> > * @mac_addr: pointer to MAC address
> > *
> > - * Tests a MAC address to ensure it is a valid Individual Address
> > + * Tests a MAC address to ensure it is a valid Address
> > **/
> > enum i40e_status_code i40e_validate_mac_addr(u8 *mac_addr) { @@
> > -980,13 +980,11 @@ enum i40e_status_code i40e_validate_mac_addr(u8
> > *mac_addr)
> >
> > DEBUGFUNC("i40e_validate_mac_addr");
> >
> > - /* Broadcast addresses ARE multicast addresses
> > -* Make sure it is not a multicast address
> > + /*
> > * Reject the zero address
> > */
> > - if (I40E_IS_MULTICAST(mac_addr) ||
> > - (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
> > - mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0))
> > + if (mac_addr[0] == 0 && mac_addr[1] == 0 && mac_addr[2] == 0 &&
> > + mac_addr[3] == 0 && mac_addr[4] == 0 && mac_addr[5] == 0)
> > status = I40E_ERR_INVALID_MAC_ADDR;
> >
> > return status;
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..00b6082 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -1199,7 +1199,8 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
> >
> > /* Get and check the mac address */
> > i40e_get_mac_addr(hw, hw->mac.addr);
> > - if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS) {
> > + if (i40e_validate_mac_addr(hw->mac.addr) != I40E_SUCCESS ||
> > + I40E_IS_MULTICAST(hw->mac.addr)) {
>
> As far as I can see this is to set PF permanent mac address during init(),
> i40e_macaddr_add() can be used to set multicast address filters.
> Why do you want to set multicast address as mac permanent address?
>
> Any chance that you want to update i40evf_add_mac_addr() to let multicast
> addresses?
No. I'm preserving the existing behavior here. The previous call used to
ensure that the permanent mac address here was both non-null and not a
multi-cast address.
Since I removed the multi cast check from i40e_validate_mac_addr() I added the
multicast check here. If "not valid" or a multicast address fail.
Regards,
Dave
>
> > PMD_INIT_LOG(ERR, "mac address is not valid");
> > ret = -EIO;
> > goto err_get_mac_addr;
> >
Re: [dpdk-dev] [PATCH] ixgbe: add counter to track sw tx packets
> -Original Message-
> From: Ananyev, Konstantin [mailto:konstantin.anan...@intel.com]
> Sent: Wednesday, August 30, 2017 10:27 AM
> To: David Harton (dharton) ; Tahhan, Maryam
>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH] ixgbe: add counter to track sw tx packets
>
>
> > >
> > > > -Original Message-
> > > > From: David Harton [mailto:dhar...@cisco.com]
> > > > Sent: Tuesday, August 29, 2017 4:51 PM
> > > > To: Ananyev, Konstantin
> > > > Cc: dev@dpdk.org; David Harton
> > > > Subject: [PATCH] ixgbe: add counter to track sw tx packets
> > > >
> > > > Add counter to track packets transmitted at the software layer to
> > > > help isolate output errors not reported otherwise.
> > >
> > >
> > > So what is the reason why same stats couldn't be collected at the
> > > application layer?
> > > I.E:
> > > nb_tx = rte_eth_tx_bulk(port, queue, ...); sw_stats[...]->tx_pkts +=
> > > nb_tx; ?
> > > That's' generic way that would work for all PMDs (not ixgbe only)
> > > and wouldn't affect actual PMD TX function in any way.
> > > Konstantin
> >
> > The motivation is to have rte_eth_stats_get() report the advertised
> > output errors along with providing some insight to where the drops are
> happening with xstats (i.e. happening after passing the DPDK boundary).
> >
> > Are you asking that DPDK track the tx stats in librte_ether so we can
> report oerrors?
>
> Yep, or even better - probably at the layer above rte_ethdev.
>
> > The problem with that (similar to tracking at the application layer)
> > is some drivers do report oerrors so how does that layer know when it
> should or shouldn't provide that stat?
>
> Something like that:
> if (stats->oerrors == 0 && sw_stats->tx_pkts > stats->opackets)
> stats->oerrors = sw_stats->tx_pkts - stats->opackets; ?
>
> >
> > If the concern is cache thrashing as Stephen implies then the stats
> could be made per Q.
>
> Yes, one is cache sharing concern.
> Though in more generic way - concern that fixing that small HW limitation
> might affect ixgbe TX performance.
Ok, so sounds like we aren't willing to tradeoff any performance to address the
problem where it lies.
I don't really want to pollute the common code for a driver specific issue.
I'll just mark this patch as Rejected.
Thanks for the feedback,
Dave
> BTW, as I remember it wasn't like that before.
> It was done by the patch:
> http://dpdk.org/ml/archives/dev/2015-July/022289.html
> even though I acked it, I don't remember what was the issue :( Maryam
> could you probably remind what was the exact problem with the old way for
> calculating oerros (via TXDGPC)?
> Thanks
> Konstantin
>
> >
> > Thanks,
> > Dave
> >
> > >
> > > >
> > > > Signed-off-by: David Harton
> > > > ---
> > > > drivers/net/ixgbe/ixgbe_ethdev.c | 98
> > > >
> > > > drivers/net/ixgbe/ixgbe_ethdev.h | 9
> > > > drivers/net/ixgbe/ixgbe_rxtx.c | 49 ++--
> > > > 3 files changed, 132 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > index ed21af5..a38a595 100644
> > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > @@ -728,6 +728,14 @@ struct rte_ixgbe_xstats_name_off { #define
> > > > IXGBE_NB_HW_STATS (sizeof(rte_ixgbe_stats_strings) / \
> > > >sizeof(rte_ixgbe_stats_strings[0]))
> > > >
> > > > +/* SW statistics */
> > > > +static const struct rte_ixgbe_xstats_name_off
> > > > +rte_ixgbe_sw_strings[] =
> > > {
> > > > + {"tx_pkts", offsetof(struct ixgbe_sw_stats, tx_pkts)}, };
> > > > +
> > > > +#define IXGBE_NB_SW_STATS (sizeof(rte_ixgbe_sw_strings) / \
> > > > + sizeof(rte_ixgbe_sw_strings[0]))
> > > > +
> > > > /* MACsec statistics */
> > > > static const struct rte_ixgbe_xstats_name_off
> > > rte_ixgbe_macsec_strings[] = {
> > > > {"out_pkts_untagged", offsetof(struct ixgbe_macsec_stats, @@
> > > > -3085,6
> > > > +3093,8 @@ static int eth_ixgb
Re: [dpdk-dev] [PATCH] ixgbe: add counter to track sw tx packets
> -Original Message-
> From: Ananyev, Konstantin [mailto:konstantin.anan...@intel.com]
> Sent: Tuesday, August 29, 2017 3:29 PM
> To: David Harton (dharton)
> Cc: dev@dpdk.org
> Subject: RE: [PATCH] ixgbe: add counter to track sw tx packets
>
>
>
> > -Original Message-
> > From: David Harton [mailto:dhar...@cisco.com]
> > Sent: Tuesday, August 29, 2017 4:51 PM
> > To: Ananyev, Konstantin
> > Cc: dev@dpdk.org; David Harton
> > Subject: [PATCH] ixgbe: add counter to track sw tx packets
> >
> > Add counter to track packets transmitted at the software layer to help
> > isolate output errors not reported otherwise.
>
>
> So what is the reason why same stats couldn't be collected at the
> application layer?
> I.E:
> nb_tx = rte_eth_tx_bulk(port, queue, ...); sw_stats[...]->tx_pkts +=
> nb_tx; ?
> That's' generic way that would work for all PMDs (not ixgbe only) and
> wouldn't affect actual PMD TX function in any way.
> Konstantin
The motivation is to have rte_eth_stats_get() report the advertised output
errors along with providing some insight to where the drops are happening with
xstats (i.e. happening after passing the DPDK boundary).
Are you asking that DPDK track the tx stats in librte_ether so we can report
oerrors? The problem with that (similar to tracking at the application layer)
is some drivers do report oerrors so how does that layer know when it should or
shouldn't provide that stat?
If the concern is cache thrashing as Stephen implies then the stats could be
made per Q.
Thanks,
Dave
>
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 98
> >
> > drivers/net/ixgbe/ixgbe_ethdev.h | 9
> > drivers/net/ixgbe/ixgbe_rxtx.c | 49 ++--
> > 3 files changed, 132 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index ed21af5..a38a595 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -728,6 +728,14 @@ struct rte_ixgbe_xstats_name_off { #define
> > IXGBE_NB_HW_STATS (sizeof(rte_ixgbe_stats_strings) / \
> >sizeof(rte_ixgbe_stats_strings[0]))
> >
> > +/* SW statistics */
> > +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_sw_strings[] =
> {
> > + {"tx_pkts", offsetof(struct ixgbe_sw_stats, tx_pkts)}, };
> > +
> > +#define IXGBE_NB_SW_STATS (sizeof(rte_ixgbe_sw_strings) / \
> > + sizeof(rte_ixgbe_sw_strings[0]))
> > +
> > /* MACsec statistics */
> > static const struct rte_ixgbe_xstats_name_off
> rte_ixgbe_macsec_strings[] = {
> > {"out_pkts_untagged", offsetof(struct ixgbe_macsec_stats, @@ -3085,6
> > +3093,8 @@ static int eth_ixgbevf_pci_remove(struct rte_pci_device
> *pci_dev)
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > struct ixgbe_hw_stats *hw_stats =
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats =
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> > struct ixgbe_macsec_stats *macsec_stats =
> > IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(
> > dev->data->dev_private);
> > @@ -3130,7 +3140,10 @@ static int eth_ixgbevf_pci_remove(struct
> rte_pci_device *pci_dev)
> > hw_stats->fclast;
> >
> > /* Tx Errors */
> > - stats->oerrors = 0;
> > + if (sw_stats->tx_pkts > stats->opackets)
> > + stats->oerrors = sw_stats->tx_pkts - stats->opackets;
> > + else
> > + stats->oerrors = 0;
> > }
> >
> > static void
> > @@ -3138,18 +3151,21 @@ static int eth_ixgbevf_pci_remove(struct
> > rte_pci_device *pci_dev) {
> > struct ixgbe_hw_stats *stats =
> > IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private);
> > + struct ixgbe_sw_stats *sw_stats =
> > + IXGBE_DEV_PRIVATE_TO_SW_STATS(dev->data->dev_private);
> >
> > /* HW registers are cleared on read */
> > ixgbe_dev_stats_get(dev, NULL);
> >
> > /* Reset software totals */
> > memset(stats, 0, sizeof(*stats));
> > + memset(sw_stats, 0, sizeof(*sw_stats));
> > }
> >
> > /* This function calculates the number of xstats based on the current
> >
Re: [dpdk-dev] [PATCH] ixgbe: add counter to track sw tx packets
> -Original Message- > From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Tuesday, August 29, 2017 2:43 PM > To: David Harton (dharton) > Cc: konstantin.anan...@intel.com; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ixgbe: add counter to track sw tx packets > > On Tue, 29 Aug 2017 11:50:56 -0400 > David Harton wrote: > > > + if (n < (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS)) > > + return (IXGBEVF_NB_XSTATS + IXGBE_NB_SW_STATS); > > Please don't use BSD style. > Return does not need parenthesis. Sure.
Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist symbols
> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Monday, August 28, 2017 9:27 AM
> To: David Harton (dharton) ;
> konstantin.anan...@intel.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: eliminate duplicate filterlist
> symbols
>
> On 8/25/2017 8:21 PM, David Harton wrote:
> > Some compilers generate warnings for duplicate symbols for the set of
> > filter lists current defined in ixgbe_ethdev.h.
> > This commits moves the defintion and declaration to the source file
> > that actually uses them and provides a function to initialize the
> > values akin to its flush function.
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 8 ++--
> > drivers/net/ixgbe/ixgbe_ethdev.h | 7 +--
> > drivers/net/ixgbe/ixgbe_flow.c | 18 ++
> > 3 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 1ec5aaf..ed21af5 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -1332,12 +1332,8 @@ struct rte_ixgbe_xstats_name_off {
> > /* initialize l2 tunnel filter list & hash */
> > ixgbe_l2_tn_filter_init(eth_dev);
> >
> > - TAILQ_INIT(&filter_ntuple_list);
> > - TAILQ_INIT(&filter_ethertype_list);
> > - TAILQ_INIT(&filter_syn_list);
> > - TAILQ_INIT(&filter_fdir_list);
> > - TAILQ_INIT(&filter_l2_tunnel_list);
> > - TAILQ_INIT(&ixgbe_flow_list);
> > + /* initialize flow filter lists */
> > + ixgbe_filterlist_init();
>
> Will it work if list initialization converted to the static
> initialization? That would remove requirement of this function call.
I was trying to keep the behavior the same:
.probe -> eth_ixgbe_dev_init -> ixgbe_filterlist_init
.remove -> eth_ixgbe_dev_uninit -> ixgbe_filterlist_flush
While I think the below would work I always like mirror/parallel relationships
like ctor/dtor for objects.
Is that reasonable?
Thanks,
Dave
>
> something like:
>
> diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> b/drivers/net/ixgbe/ixgbe_flow.c index c8645f056..6184eb4af 100644
> --- a/drivers/net/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> @@ -79,6 +79,24 @@
> #define IXGBE_MAX_N_TUPLE_PRIO 7
> #define IXGBE_MAX_FLX_SOURCE_OFF 62
>
> +static TAILQ_HEAD(ixgbe_ntuple_filter_list, ixgbe_ntuple_filter_ele)
> + filter_ntuple_list = TAILQ_HEAD_INITIALIZER(filter_ntuple_list);
> +
> +static TAILQ_HEAD(ixgbe_ethertype_filter_list,
> ixgbe_ethertype_filter_ele)
> + filter_ethertype_list =
> TAILQ_HEAD_INITIALIZER(filter_ethertype_list);
> +
> +static TAILQ_HEAD(ixgbe_syn_filter_list, ixgbe_eth_syn_filter_ele)
> + filter_syn_list = TAILQ_HEAD_INITIALIZER(filter_syn_list);
> +
> +static TAILQ_HEAD(ixgbe_fdir_rule_filter_list, ixgbe_fdir_rule_ele)
> + filter_fdir_list = TAILQ_HEAD_INITIALIZER(filter_fdir_list);
> +
> +static TAILQ_HEAD(ixgbe_l2_tunnel_filter_list,
> ixgbe_eth_l2_tunnel_conf_ele)
> + filter_l2_tunnel_list =
> TAILQ_HEAD_INITIALIZER(filter_l2_tunnel_list);
> +
> +static TAILQ_HEAD(ixgbe_flow_mem_list, ixgbe_flow_mem)
> + ixgbe_flow_list = TAILQ_HEAD_INITIALIZER(ixgbe_flow_list);
> +
Re: [dpdk-dev] [PATCH] vmxnet3: replenish ring buffers in rx processing
> -Original Message-
> From: Wiles, Keith [mailto:keith.wi...@intel.com]
> Sent: Friday, August 25, 2017 11:41 AM
> To: David Harton (dharton)
> Cc: skh...@vmware.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] vmxnet3: replenish ring buffers in rx
> processing
>
>
> > On Aug 25, 2017, at 10:22 AM, David Harton wrote:
> >
> > vmxnet3 rx processing should replenish ring buffers after new buffers
> > are available to prevent the interface from getting stuck in a state
> > that no new work is processed.
> >
> > Signed-off-by: David Harton
> > ---
> > drivers/net/vmxnet3/vmxnet3_rxtx.c | 17 +
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > index d9cf437..9861d35 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > @@ -880,6 +880,23 @@
> > }
> > }
> >
> > + if (unlikely(nb_rxd == 0)) {
> > + uint32_t avail;
> > + for (ring_idx = 0; ring_idx < VMXNET3_RX_CMDRING_SIZE;
> ring_idx++) {
> > + avail = vmxnet3_cmd_ring_desc_avail(&rxq-
> >cmd_ring[ring_idx]);
> > + if (unlikely(avail > 0)) {
> > + /* try to alloc new buf and renew descriptors */
> > + vmxnet3_post_rx_bufs(rxq, ring_idx);
> > + }
> > + }
> > + if (unlikely(rxq->shared->ctrl.updateRxProd)) {
> > + for (ring_idx = 0; ring_idx < VMXNET3_RX_CMDRING_SIZE;
> ring_idx++) {
> > + VMXNET3_WRITE_BAR0_REG(hw, rxprod_reg[ring_idx]
> > +
> (rxq->queue_id
> > +* VMXNET3_REG_ALIGN),
>
> Did you run checkpatch here as it seems the line length is greater then
> 80.
I did. I'm following the conventions already established in the file.
Several lines above the file are > 80 characters. In fact, some just a few
lines above these diffs.
Are you asking me to deviate from the established code convention?
Thanks,
Dave
>
> BTW, I think requirements of 80 and tab length of 8, is a bit outdated in
> 2017 :-(
>
> > + rxq-
> >cmd_ring[ring_idx].next2fill);
> > + }
> > + }
> > + }
> > +
> > return nb_rx;
> > }
> >
> > --
> > 1.8.3.1
> >
>
> Regards,
> Keith
Re: [dpdk-dev] [PATCH] ethdev: modifiy vlan_offload_set_t to return int
> -Original Message-
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Thursday, August 24, 2017 7:37 PM
> To: David Harton (dharton)
> Cc: tho...@monjalon.net; ferruh.yi...@intel.com;
> stephen.h...@broadcom.com; ajit.khapa...@broadcom.com; John Daley
> (johndale) ; wenzhuo...@intel.com;
> konstantin.anan...@intel.com; jingjing...@intel.com;
> beilei.x...@intel.com; jing.d.c...@intel.com; adrien.mazarg...@6wind.com;
> nelio.laranje...@6wind.com; alejandro.luc...@netronome.com;
> hemant.agra...@nxp.com; rasesh.m...@cavium.com; harish.pa...@cavium.com;
> skh...@vmware.com; y...@fridaylinux.org; maxime.coque...@redhat.com;
> allain.leg...@windriver.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: modifiy vlan_offload_set_t to
> return int
>
> On Thu, 24 Aug 2017 19:18:51 -0400
> David Harton wrote:
>
> > @@ -2031,7 +2031,7 @@ struct avp_queue {
> > mask = (ETH_VLAN_STRIP_MASK |
> > ETH_VLAN_FILTER_MASK |
> > ETH_VLAN_EXTEND_MASK);
> > - avp_vlan_offload_set(eth_dev, mask);
> > + (void)avp_vlan_offload_set(eth_dev, mask);
>
> This is a BSDism. You don't need the void cast.
Never know what to do...SA tools and some compilers whine if I don't.
People complain if I do. :)
What if I check the return code and log an error?
Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
> -Original Message- > From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Wednesday, August 23, 2017 5:57 PM > To: David Harton (dharton) > Cc: tho...@monjalon.net; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf > by rte_eth_stats_get > > On Tue, 22 Aug 2017 22:55:55 -0400 > David Harton wrote: > > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the > > device was setting the value. A check has been added in > > rte_eth_stats_get() to leave the device value in-tact when non-zero. > > > > Signed-off-by: David Harton > > --- > > > > v2: Fixed braces complaint required by other coding standards. > > > > lib/librte_ether/rte_ethdev.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > b/lib/librte_ether/rte_ethdev.c index 0597641..0a1d3b8 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1336,8 +1336,11 @@ struct rte_eth_dev * > > memset(stats, 0, sizeof(*stats)); > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP); > > - stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; > > (*dev->dev_ops->stats_get)(dev, stats); > > + /* only set rx_nombuf if not set by the device */ > > + if (!stats->rx_nombuf) > > + stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; > > + > > return 0; > > } > > > > This seems backwards. It seems like the original way worked fine. > If device specific code wanted to override rx_nombuf it could do so either > by adding it's additional value or just setting rx_nombuf. > > Adding special cases seems like it would start a bad precedent and the > could would end up quite complex as some values had one semantic and > others were only from driver. Eternal apologies. This is another example of me trying to upstream a fix we've held on to for far too long and not realizing it has been addressed. I see that this was fixed here: 53ecfa24fbcd17d9855937391ce347f37434fbfa Again, apologies...I'll be careful publishing any further fixes trying to determine if others have fixed in different ways. Regards, Dave
Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
> -Original Message- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Wednesday, August 23, 2017 9:24 AM > To: David Harton (dharton) > Cc: dev@dpdk.org > Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by > rte_eth_stats_get > > 23/08/2017 14:18, David Harton (dharton): > > > > > -Original Message- > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > Sent: Wednesday, August 23, 2017 3:52 AM > > > To: David Harton (dharton) > > > Cc: dev@dpdk.org > > > Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by > > > rte_eth_stats_get > > > > > > 23/08/2017 04:55, David Harton: > > > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the > > > > device was setting the value. A check has been added in > > > > rte_eth_stats_get() to leave the device value in-tact when non-zero. > > > > > > If we get this counter from stats->rx_nombuf, why keeping > > > dev->data->rx_mbuf_alloc_failed ? > > > We could rework other PMDs to not use this global variable. > > > It is inconsistent to use it for some PMDs but not all. > > > And it seems not used outside of PMDs. > > > > Are you also asking to remove dev->data->rx_mbuf_alloc_failed as well > since we will have an ABI breakage anyway? > > Not asking, just giving my thought :) I did some more digging. For this count it looks like some devices: - have their own internal version - have a count shared with the pf - rely on this field to maintain the count - don't count this failure at all :( With that said I'd like to keep with the requested changes. Thoughts? Dave > > > On an somewhat related note, since we are introducing an ABI breakage > how do you feel about re-adding the return code for the vlan_offload_set > vector? Some devices conditionally provide the ability to modify some > offload and the caller should know. Since I've got your attention thought > I'd ask here before posting the patch. > > Seems reasonnable > > > > > In fact, I believe all the API function calls should provide a return > code to help mitigate ABI breakages and also provide the ability to let > the caller distinguish between - no device, not supported and some other > error. A control plane often needs to understand these distinctions to > properly orchestrate the system and/or report real errors. This is more > than I'm willing to take on myself but believe it's a principle I'd like > to discuss (can start separate thread if desired). > > > > Yes you're right
Re: [dpdk-dev] [PATCH] ena: fix init of ena pci_dev info
> -Original Message- > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] > Sent: Wednesday, August 23, 2017 6:48 AM > To: David Harton (dharton) ; m...@semihalf.com; > m...@semihalf.com; gtza...@amazon.com; evge...@amazon.com > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ena: fix init of ena pci_dev info > > On 8/23/2017 1:41 AM, David Harton wrote: > > eth_ena_dev_init() was not initializing all of the common pci dev info > > for the rte_eth_dev. Added call to > > rte_eth_copy_pci_info() to complete the init particularly the driver > > name. > > rte_eth_copy_pci_info() already called during probe [1], what information > was missing? > > [1] > eth_ena_pci_probe() > rte_eth_dev_pci_generic_probe() > rte_eth_dev_pci_allocate() > rte_eth_copy_pci_info() <--- > eth_ena_dev_init() > ... <--- where rte_eth_copy_pci_info() added again. This patch was based on 16.07. It looks like "recent" change added the missing coverage and I missed it: dcd5c8112bc3d0c8407cbab1f3a16799a8c7e5e4 Thanks, Dave > > > > > Signed-off-by: David Harton > > --- > > drivers/net/ena/ena_ethdev.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ena/ena_ethdev.c > > b/drivers/net/ena/ena_ethdev.c index 80ce1f3..a6c408b 100644 > > --- a/drivers/net/ena/ena_ethdev.c > > +++ b/drivers/net/ena/ena_ethdev.c > > @@ -1289,6 +1289,7 @@ static int eth_ena_dev_init(struct rte_eth_dev > *eth_dev) > > return 0; > > > > pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev); > > + rte_eth_copy_pci_info(eth_dev, pci_dev); > > adapter->pdev = pci_dev; > > > > PMD_INIT_LOG(INFO, "Initializing %x:%x:%x.%d", > >
Re: [dpdk-dev] [PATCH] ixgbe: initialize scattered_rx during dev_configure
> -Original Message- > From: Ananyev, Konstantin [mailto:konstantin.anan...@intel.com] > Sent: Wednesday, August 23, 2017 6:32 AM > To: David Harton (dharton) ; wenzhuo...@intel.co > Cc: dev@dpdk.org > Subject: RE: [PATCH] ixgbe: initialize scattered_rx during dev_configure > > Hi David, > > > > > An application may want to manipulate the MTU settings of a device > > without having to start the device first. > > In order to remove the need to start the device the ixgbe/ixgbevf > > drivers need to initialize the scattered_rx value during > > dev_configure. > > Not sure how that would help? > As I can see ixgbe_dev_mtu_set() uses rx_conf->enable_scatter to check is > multiseg RX allowed or not. Hi Konstantin, I see. The problem has been fixed already. Originally (v2.2), the check in ixgbe_dev_mtu_set() was made against dev->data->scattered_rx but it has been changed. Sorry, I'll rescind the request. Thanks! Dave > Konstantin > > > > > Signed-off-by: David Harton > > --- > > drivers/net/ixgbe/ixgbe_ethdev.c | 14 ++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > index 22171d8..e85bdb4 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > @@ -2372,6 +2372,13 @@ static int eth_ixgbevf_pci_remove(struct > rte_pci_device *pci_dev) > > intr->flags |= IXGBE_FLAG_NEED_LINK_UPDATE; > > > > /* > > +* Update scattered_rx so we can update MTU immediately > > +* following configure without having to start the device > > +*/ > > + if (dev->data->dev_conf.rxmode.enable_scatter) > > + dev->data->scattered_rx = 1; > > + > > + /* > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > > * allocation or vector Rx preconditions we will reset it. > > */ > > @@ -4949,6 +4956,13 @@ static int > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev, > > #endif > > > > /* > > +* Update scattered_rx so we can update MTU immediately > > +* following configure without having to start the device > > +*/ > > + if (dev->data->dev_conf.rxmode.enable_scatter) > > + dev->data->scattered_rx = 1; > > + > > + /* > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > > * allocation or vector Rx preconditions we will reset it. > > */ > > -- > > 1.8.3.1
Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
> -Original Message- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Wednesday, August 23, 2017 3:52 AM > To: David Harton (dharton) > Cc: dev@dpdk.org > Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by > rte_eth_stats_get > > 23/08/2017 04:55, David Harton: > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the > > device was setting the value. A check has been added in > > rte_eth_stats_get() to leave the device value in-tact when non-zero. > > If we get this counter from stats->rx_nombuf, why keeping > dev->data->rx_mbuf_alloc_failed ? > We could rework other PMDs to not use this global variable. > It is inconsistent to use it for some PMDs but not all. > And it seems not used outside of PMDs. Are you also asking to remove dev->data->rx_mbuf_alloc_failed as well since we will have an ABI breakage anyway? On an somewhat related note, since we are introducing an ABI breakage how do you feel about re-adding the return code for the vlan_offload_set vector? Some devices conditionally provide the ability to modify some offload and the caller should know. Since I've got your attention thought I'd ask here before posting the patch. In fact, I believe all the API function calls should provide a return code to help mitigate ABI breakages and also provide the ability to let the caller distinguish between - no device, not supported and some other error. A control plane often needs to understand these distinctions to properly orchestrate the system and/or report real errors. This is more than I'm willing to take on myself but believe it's a principle I'd like to discuss (can start separate thread if desired).
Re: [dpdk-dev] [PATCH v3] ethdev: add return code to rte_eth_stats_reset()
> -Original Message-
> From: Van Haaren, Harry [mailto:harry.van.haa...@intel.com]
> Sent: Tuesday, August 08, 2017 5:03 AM
> To: David Harton (dharton) ; tho...@monjalon.net
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3] ethdev: add return code to
> rte_eth_stats_reset()
>
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of David Harton
> > Sent: Monday, August 7, 2017 6:39 PM
> > To: tho...@monjalon.net
> > Cc: dev@dpdk.org; David Harton
> > Subject: [dpdk-dev] [PATCH v3] ethdev: add return code to
> > rte_eth_stats_reset()
> >
> > Some devices do not support reset of eth stats. An application may
> > need to know not to clear shadow stats if the device cannot.
> >
> > rte_eth_stats_reset is updated to provide a return code to share
> > whether the device supports reset or not.
> >
> > Signed-off-by: David Harton
> > ---
>
> Hi,
>
> As far as I know changing the return type (void to int) of a function does
> *not* break ABI, but does "break" API as the application code should now
> check the return value. In theory the application could ignore the return
> value and current behavior is maintained.
>
> The validate-abi.sh script says "Compatible" with the following item
> flagged:
>
> Problems with Symbols
> High 0
> Medium 0
> Low 1
>
> Change>> Type of return value has been changed from void to int (4 bytes).
> Effect>> Replacement of return type may indicate a change in its semantic
> meaning.
>
> Perhaps somebody with more ABI expertise than I would double check the
> return value change?
>
>
> Some smaller things inline below.
>
> > v3:
> > * overcame noob errors and figured out patch challenges
> > * this release should finally be clean :)
> >
> > v2:
> > * fixed soft tab issue inserted while moving changes
> >
> > lib/librte_ether/rte_ethdev.c | 8 +---
> > lib/librte_ether/rte_ethdev.h | 4 +++-
> > 2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 0597641..f72cc5a 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1341,17 +1341,19 @@ rte_eth_stats_get(uint8_t port_id, struct
> rte_eth_stats *stats)
> > return 0;
> > }
> >
> > -void
> > +int
> > rte_eth_stats_reset(uint8_t port_id)
> > {
> > struct rte_eth_dev *dev;
> >
> > - RTE_ETH_VALID_PORTID_OR_RET(port_id);
> > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > dev = &rte_eth_devices[port_id];
> >
> > - RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset);
> > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_reset, -ENOTSUP);
> > (*dev->dev_ops->stats_reset)(dev);
> > dev->data->rx_mbuf_alloc_failed = 0;
> > +
> > + return 0;
> > }
> >
> > static int
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index 0adf327..d8ccd60 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2246,8 +2246,10 @@ int rte_eth_stats_get(uint8_t port_id, struct
> > rte_eth_stats *stats);
> > *
> > * @param port_id
> > * The port identifier of the Ethernet device.
> > + * @return
> > + * Zero if successful. Non-zero otherwise.
>
> We should document all return values:
>
> @retval 0 Success
> @retval -EINVAL Invalid port_id provided @retval -ENOTSUP Stats reset
> functionality not supported by PMD
Sure. I was following the convention of function above it
rte_eth_stats_get() but I agree better to advertise externally.
I'll also modify the port number check errval to -ENODEV.
>
> The API change itself should probably be added to release notes, as
> applications may wish to be aware this function has changed.
Sounds good.
>
> > */
> > -void rte_eth_stats_reset(uint8_t port_id);
> > +int rte_eth_stats_reset(uint8_t port_id);
> >
> > /**
> > * Retrieve names of extended statistics of an Ethernet device.
> > --
> > 2.10.3.dirty
>
>
> I'm happy to Ack the code/release-notes with above suggestions, but I'd
> like a second opinion to Ack ABI.
Thanks for the review,
Dave
>
> -Harry
[dpdk-dev] [PATCH] net/enic: heed VLAN strip flag in device configure function
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Daley (johndale)
> Sent: Tuesday, July 19, 2016 6:44 PM
> To: dev at dpdk.org
> Cc: bruce.richardson at intel.com; John Daley (johndale) cisco.com>
> Subject: [dpdk-dev] [PATCH] net/enic: heed VLAN strip flag in device
> configure function
>
> The configure function enicpmd_dev_configure() was not paying attention to
> the rxmode VLAN strip bit. Set the VLAN strip mode according to the bit.
>
> Fixes: fefed3d1e62c ("enic: new driver")
>
> Signed-off-by: John Daley
> ---
>
> Although not critical, it is low risk and would be nice to have in 16_07.
>
> drivers/net/enic/enic_ethdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/enic/enic_ethdev.c
> b/drivers/net/enic/enic_ethdev.c index 59082d2..47b07c9 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -356,6 +356,7 @@ static int enicpmd_dev_configure(struct rte_eth_dev
> *eth_dev)
> eth_dev->data->dev_conf.rxmode.split_hdr_size);
> }
>
> + enicpmd_vlan_offload_set(eth_dev, ETH_VLAN_STRIP_MASK);
> enic->hw_ip_checksum = eth_dev->data-
> >dev_conf.rxmode.hw_ip_checksum;
> return 0;
> }
> --
> 2.7.0
Reviewed-by: David Harton
Tested-by: David Harton
[dpdk-dev] [PATCH v3 00/10] Remove string operations from xstats
Acked-by: David Harton > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton > Sent: Monday, May 30, 2016 6:48 AM > To: dev at dpdk.org; Thomas Monjalon ; Helin > Zhang > ; Wenzhuo Lu ; Jing Chen > ; Huawei Xie > Subject: [dpdk-dev] [PATCH v3 00/10] Remove string operations from xstats > > The current extended ethernet statistics fetching involve doing several > string operations, which causes performance issues if there are lots of > statistics and/or network interfaces. This patchset changes the API for > xstats to use integer identifiers instead of strings and implements > this new API for the ixgbe, i40e, e1000, fm10k, and virtio drivers. > > -- > > v3 changes: > * Corrected ixgbe vf xstats fetching > * Added xstats changes to e1000, f10k, and virtio drivers > * Added cleanup patch that removes now-redundant name field > * Removed ethtool xstats command > * Removed unused .xstats_count from eth-dev_ops > * Changed test-pmd & proc_info to use new API > * Added documentation update > * Added missing changes to .map file (affected shared lib builds) > > v2 changes: > * Fetching xstats count now seperate API function > * Added #define constants for some magic numbers > * Fixed bug with virtual function count fetching > * For non-xstats-supporting drivers, queue stats returned > * Some refactoring/cleanups > * Removed index assumption from example > > Remy Horton (10): > rte: change xstats to use integer ids > drivers/net/ixgbe: change xstats to use integer ids > drivers/net/e1000: change xstats to use integer ids > drivers/net/fm10k: change xstats to use integer ids > drivers/net/i40e: change xstats to use integer ids > drivers/net/virtio: change xstats to use integer ids > app/test-pmd: change xstats to use integer ids > app/proc_info: change xstats to use integer ids > remove name field from struct rte_eth_xstats > doc: update xstats documentation > > app/proc_info/main.c| 26 - > app/test-pmd/config.c | 52 + > doc/guides/prog_guide/poll_mode_drv.rst | 25 +++-- > drivers/net/e1000/igb_ethdev.c | 50 +++-- > drivers/net/fm10k/fm10k_ethdev.c| 52 ++--- > drivers/net/i40e/i40e_ethdev.c | 77 +- > drivers/net/i40e/i40e_ethdev_vf.c | 24 +++- > drivers/net/ixgbe/ixgbe_ethdev.c| 98 > - > drivers/net/virtio/virtio_ethdev.c | 60 +--- > lib/librte_ether/rte_ethdev.c | 92 > --- > lib/librte_ether/rte_ethdev.h | 44 ++- > lib/librte_ether/rte_ether_version.map | 7 +++ > 12 files changed, 527 insertions(+), 80 deletions(-) > > -- > 2.5.5
[dpdk-dev] [RFC PATCH v2 3/3] examples/ethtool: add xstats display command
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, May 06, 2016 7:11 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [RFC PATCH v2 3/3] examples/ethtool: add xstats
> display command
>
> Signed-off-by: Remy Horton
> ---
> examples/ethtool/ethtool-app/ethapp.c | 57
> +++
> 1 file changed, 57 insertions(+)
>
> diff --git a/examples/ethtool/ethtool-app/ethapp.c
> b/examples/ethtool/ethtool-app/ethapp.c
> index 2ed4796..1dc0c35 100644
> --- a/examples/ethtool/ethtool-app/ethapp.c
> +++ b/examples/ethtool/ethtool-app/ethapp.c
> @@ -98,6 +98,8 @@ cmdline_parse_token_string_t pcmd_rxmode_token_cmd =
> TOKEN_STRING_INITIALIZER(struct pcmd_int_params, cmd, "rxmode");
> cmdline_parse_token_string_t pcmd_portstats_token_cmd =
> TOKEN_STRING_INITIALIZER(struct pcmd_int_params, cmd, "portstats");
> +cmdline_parse_token_string_t pcmd_xstats_token_cmd =
> + TOKEN_STRING_INITIALIZER(struct pcmd_int_params, cmd, "xstats");
> cmdline_parse_token_num_t pcmd_int_token_port =
> TOKEN_NUM_INITIALIZER(struct pcmd_int_params, port, UINT16);
>
> @@ -552,6 +554,49 @@ static void pcmd_portstats_callback(__rte_unused void
> *ptr_params,
> printf("Port %i: Error fetching statistics\n", params->port);
> }
>
> +static void pcmd_xstats_callback(__rte_unused void *ptr_params,
> + __rte_unused struct cmdline *ctx,
> + __rte_unused void *ptr_data)
> +{
> + struct rte_eth_xstats xstats[256];
> + struct pcmd_int_params *params = ptr_params;
> + int cnt_xstats, idx_xstat, idx_name;
> + struct rte_eth_xstats_name *ptr_names;
> +
> + if (!rte_eth_dev_is_valid_port(params->port)) {
> + printf("Error: Invalid port number %i\n", params->port);
> + return;
> + }
> +
> + cnt_xstats = rte_eth_xstats_count(params->port);
> + if (cnt_xstats < 0) {
> + printf("Port %i: %s\n", params->port, strerror(-cnt_xstats));
> + return;
> + }
> + printf("Number of xstats: %i\n", cnt_xstats);
> + ptr_names = malloc(sizeof(struct rte_eth_xstats_name) * cnt_xstats);
> + if (cnt_xstats != rte_eth_xstats_names(
> + params->port, ptr_names, cnt_xstats)) {
> + printf("Error: Fetched and expected counts mismatch\n");
> + return;
> + }
> +
> + cnt_xstats = rte_eth_xstats_get(params->port, xstats, 256);
> + if (cnt_xstats < 0) {
> + printf("Error: Unable to get xstats (%s)\n",
> + strerror(-cnt_xstats));
> + return;
> + }
> + for (idx_xstat = 0; idx_xstat < cnt_xstats; idx_xstat++)
> + for (idx_name = 0; idx_name < cnt_xstats; idx_name++)
> + if (ptr_names[idx_name].id == xstats[idx_xstat].id) {
> + printf("%s: %lu\n", ptr_names[idx_name].name,
> + xstats[idx_xstat].value);
> + break;
> + }
> + free(ptr_names);
> +}
> +
> static void pcmd_ringparam_callback(__rte_unused void *ptr_params,
> __rte_unused struct cmdline *ctx,
> void *ptr_data)
> @@ -790,6 +835,17 @@ cmdline_parse_inst_t pcmd_portstats = {
> NULL
> },
> };
> +cmdline_parse_inst_t pcmd_xstats = {
> + .f = pcmd_xstats_callback,
> + .data = NULL,
> + .help_str = "xstats \n"
> + " Print port eth xstats",
> + .tokens = {
> + (void *)&pcmd_xstats_token_cmd,
> + (void *)&pcmd_int_token_port,
> + NULL
> + },
> +};
> cmdline_parse_inst_t pcmd_ringparam = {
> .f = pcmd_ringparam_callback,
> .data = NULL,
> @@ -858,6 +914,7 @@ cmdline_parse_ctx_t list_prompt_commands[] = {
> (cmdline_parse_inst_t *)&pcmd_stop,
> (cmdline_parse_inst_t *)&pcmd_validate,
> (cmdline_parse_inst_t *)&pcmd_vlan,
> + (cmdline_parse_inst_t *)&pcmd_xstats,
> (cmdline_parse_inst_t *)&pcmd_quit,
> NULL
> };
> --
> 2.5.5
Acked-by: David Harton
[dpdk-dev] [RFC PATCH v2 2/3] drivers/net/ixgbe: change xstats to use integer id
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, May 06, 2016 7:11 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [RFC PATCH v2 2/3] drivers/net/ixgbe: change xstats to
> use integer id
>
> Signed-off-by: Remy Horton
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 98 ++-
> -
> 1 file changed, 85 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3f1ebc1..3ff8bae 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -179,6 +179,10 @@ static int ixgbevf_dev_xstats_get(struct rte_eth_dev
> *dev,
> struct rte_eth_xstats *xstats, unsigned n);
> static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
> static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
> +static int ixgbe_dev_xstats_names(__rte_unused struct rte_eth_dev *dev,
> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit);
> +static int ixgbevf_dev_xstats_names(__rte_unused struct rte_eth_dev *dev,
> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit);
> static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
>uint16_t queue_id,
>uint8_t stat_idx,
> @@ -466,6 +470,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
> .xstats_get = ixgbe_dev_xstats_get,
> .stats_reset = ixgbe_dev_stats_reset,
> .xstats_reset = ixgbe_dev_xstats_reset,
> + .xstats_names = ixgbe_dev_xstats_names,
> .queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
> .dev_infos_get= ixgbe_dev_info_get,
> .dev_supported_ptypes_get = ixgbe_dev_supported_ptypes_get,
> @@ -555,6 +560,7 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops =
> {
> .xstats_get = ixgbevf_dev_xstats_get,
> .stats_reset = ixgbevf_dev_stats_reset,
> .xstats_reset = ixgbevf_dev_stats_reset,
> + .xstats_names = ixgbevf_dev_xstats_names,
> .dev_close= ixgbevf_dev_close,
> .allmulticast_enable = ixgbevf_dev_allmulticast_enable,
> .allmulticast_disable = ixgbevf_dev_allmulticast_disable,
> @@ -685,6 +691,7 @@ static const struct rte_ixgbe_xstats_name_off
> rte_ixgbe_rxq_strings[] = {
>
> #define IXGBE_NB_RXQ_PRIO_STATS (sizeof(rte_ixgbe_rxq_strings) / \
> sizeof(rte_ixgbe_rxq_strings[0]))
> +#define IXGBE_NB_RXQ_PRIO_VALUES 8
>
> static const struct rte_ixgbe_xstats_name_off rte_ixgbe_txq_strings[] = {
> {"xon_packets", offsetof(struct ixgbe_hw_stats, pxontxc)},
> @@ -695,6 +702,7 @@ static const struct rte_ixgbe_xstats_name_off
> rte_ixgbe_txq_strings[] = {
>
> #define IXGBE_NB_TXQ_PRIO_STATS (sizeof(rte_ixgbe_txq_strings) / \
> sizeof(rte_ixgbe_txq_strings[0]))
> +#define IXGBE_NB_TXQ_PRIO_VALUES 8
>
> static const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[]
> = {
> {"rx_multicast_packets", offsetof(struct ixgbevf_hw_stats, vfmprc)},
> @@ -2694,8 +2702,75 @@ ixgbe_dev_stats_reset(struct rte_eth_dev *dev)
> /* This function calculates the number of xstats based on the current
> config */
> static unsigned
> ixgbe_xstats_calc_num(void) {
> - return IXGBE_NB_HW_STATS + (IXGBE_NB_RXQ_PRIO_STATS * 8) +
> - (IXGBE_NB_TXQ_PRIO_STATS * 8);
> + return IXGBE_NB_HW_STATS +
> + (IXGBE_NB_RXQ_PRIO_STATS * IXGBE_NB_RXQ_PRIO_VALUES) +
> + (IXGBE_NB_TXQ_PRIO_STATS * IXGBE_NB_TXQ_PRIO_VALUES);
Nice cleanup.
> +}
> +
> +static int ixgbe_dev_xstats_names(__rte_unused struct rte_eth_dev *dev,
> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit)
> +{
> + const unsigned cnt_stats = ixgbe_xstats_calc_num();
> + unsigned stat, i, count;
> +
> + if (ptr_names != NULL) {
> + count = 0;
> +
> + /* Note: limit >= cnt_stats checked upstream
> + * in rte_eth_xstats_names()
> + */
> +
> + /* Extended stats from ixgbe_hw_stats */
> + for (i = 0; i < IXGBE_NB_HW_STATS; i++) {
> + ptr_names[count].id = count;
> + snprintf(ptr_names[count].name,
> + sizeof(ptr_names[count].name),
> + "%s",
> + rte_ixgbe_stats_strings[i].name);
> + count++;
> + }
> +
> + /* RX Priority Stats */
> + for (stat = 0; stat < IXGBE_NB_RXQ_PRIO_STATS; stat++) {
> + for (i = 0; i < IXGBE_NB_RXQ_PRIO_VALUES; i++) {
> + ptr_names[count].id = count;
> + snprintf(ptr_names[count].name,
> +
[dpdk-dev] [RFC PATCH v2 1/3] rte: change xstats to use integer keys
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, May 06, 2016 7:11 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [RFC PATCH v2 1/3] rte: change xstats to use integer
> keys
>
> Signed-off-by: Remy Horton
> ---
> lib/librte_ether/rte_ethdev.c | 100 -
> -
> lib/librte_ether/rte_ethdev.h | 55 +++
> 2 files changed, 142 insertions(+), 13 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a31018e..29ba12c 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -112,7 +112,6 @@ static const struct rte_eth_xstats_name_off
> rte_txq_stats_strings[] = {
> #define RTE_NB_TXQ_STATS (sizeof(rte_txq_stats_strings) /\
> sizeof(rte_txq_stats_strings[0]))
>
> -
> /**
> * The user application callback description.
> *
> @@ -1507,6 +1506,87 @@ rte_eth_stats_reset(uint8_t port_id)
> dev->data->rx_mbuf_alloc_failed = 0;
> }
>
> +int
> +rte_eth_xstats_count(uint8_t port_id)
> +{
> + struct rte_eth_dev *dev;
> + int count;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> + dev = &rte_eth_devices[port_id];
> + if (dev->dev_ops->xstats_names != NULL) {
> + count = (*dev->dev_ops->xstats_names)(dev, NULL, 0);
> + if (count < 0)
> + return count;
> + } else
> + count = 0;
> + count += RTE_NB_STATS;
> + count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS;
> + count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS;
> + return count;
> +}
> +
> +int
> +rte_eth_xstats_names(uint8_t port_id, struct rte_eth_xstats_name
> *ptr_names,
> + unsigned limit)
> +{
> + struct rte_eth_dev *dev;
> + int cnt_used_entries;
> + int cnt_expected_entries;
> + uint32_t idx, id_queue;
> +
> + if (ptr_names == NULL)
> + return -EINVAL;
> + cnt_expected_entries = rte_eth_xstats_count(port_id);
> + if (cnt_expected_entries < 0)
> + return cnt_expected_entries;
> + if ((int)limit < cnt_expected_entries)
> + return -ERANGE;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> + dev = &rte_eth_devices[port_id];
> + if (dev->dev_ops->xstats_names != NULL) {
> + cnt_used_entries = (*dev->dev_ops->xstats_names)(
> + dev, ptr_names, limit);
> + if (cnt_used_entries < 0)
> + return cnt_used_entries;
> + } else
> + /* Driver itself does not support extended stats, but
> + * still have basic stats.
> + */
> + cnt_used_entries = 0;
> +
> + for (idx = 0; idx < RTE_NB_STATS; idx++) {
> + ptr_names[cnt_used_entries].id = cnt_used_entries;
> + snprintf(ptr_names[cnt_used_entries].name,
> + sizeof(ptr_names[0].name),
> + "%s", rte_stats_strings[idx].name);
> + cnt_used_entries++;
> + }
> + for (id_queue = 0; id_queue < dev->data->nb_rx_queues; id_queue++) {
> + for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
> + ptr_names[cnt_used_entries].id = cnt_used_entries;
> + snprintf(ptr_names[cnt_used_entries].name,
> + sizeof(ptr_names[0].name),
> + "rx_q%u%s",
> + id_queue, rte_rxq_stats_strings[idx].name);
> + cnt_used_entries++;
> + }
> +
> + }
> + for (id_queue = 0; id_queue < dev->data->nb_tx_queues; id_queue++) {
> + for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
> + ptr_names[cnt_used_entries].id = cnt_used_entries;
> + snprintf(ptr_names[cnt_used_entries].name,
> + sizeof(ptr_names[0].name),
> + "tx_q%u%s",
> + id_queue, rte_txq_stats_strings[idx].name);
> + cnt_used_entries++;
> + }
> + }
> + return cnt_used_entries;
> +}
> +
> /* retrieve ethdev extended statistics */ int
> rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats, @@ -
> 1551,8 +1631,8 @@ rte_eth_xstats_get(uint8_t port_id, struct
> rte_eth_xstats *xstats,
> stats_ptr = RTE_PTR_ADD(ð_stats,
> rte_stats_strings[i].offset);
> val = *stats_ptr;
> - snprintf(xstats[count].name, sizeof(xstats[count].name),
> - "%s", rte_stats_strings[i].name);
> + xstats[count].name[0] = '\0';
> + xstats[count].id = count + xcount;
> xstats[count++].value = val;
> }
>
> @@ -1563,9 +1643,8 @@ rte_eth_xstats_get(uint8_t port_id, struct
> rte_eth_xstats *xstats,
>
[dpdk-dev] [RFC PATCH v1 2/3] drivers/net/ixgbe: change xstats to use integers
> -Original Message-
> From: Remy Horton [mailto:remy.horton at intel.com]
> Sent: Tuesday, May 03, 2016 8:23 AM
> To: David Harton (dharton) ; dev at dpdk.org; Helin
> Zhang
>
> Subject: Re: [dpdk-dev] [RFC PATCH v1 2/3] drivers/net/ixgbe: change
> xstats to use integers
>
>
> On 29/04/2016 14:43, David Harton (dharton) wrote:
> [..]
> >> + /* RX Priority Stats */
> >> + for (stat = 0; stat < IXGBE_NB_RXQ_PRIO_STATS; stat++) {
> >> + for (i = 0; i < 8; i++) {
> >
> > 8 seems magical. Is there a constant somewhere that can be used?
>
> Not that I'm aware of. I've made it a #define as a small cleanup.
>
>
> >> +static int ixgbevf_dev_xstats_names(__rte_unused struct rte_eth_dev
> *dev,
> >> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit)
> >> +{
> >> + unsigned i;
> >> +
> >> + if (limit < IXGBEVF_NB_XSTATS)
> >
> > Think this check has to be removed since rte_eth_xstats_count() calls
> > with limit == 0.
>
> Well spotted. It should only error-out if ptr_names is non-NULL.
>
> As an aside, I am wondering whether getting stats counts should itself be
> a seperate driver hook.
I personally think it would be cleaner. Bug, I can also see the argument of
keeping
the vector table smaller and would conceded to others that felt strongly about
it.
I do really like having a separate external API to the user. A user should
explicitly
ask for the count.
Cheers,
Dave
>
>
> ..Remy
[dpdk-dev] [RFC PATCH v1 2/3] drivers/net/ixgbe: change xstats to use integers
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, April 15, 2016 10:44 AM
> To: dev at dpdk.org; Helin Zhang
> Subject: [dpdk-dev] [RFC PATCH v1 2/3] drivers/net/ixgbe: change xstats to
> use integers
>
> Signed-off-by: Remy Horton
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 87
> +++-
> 1 file changed, 78 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3f1ebc1..4d31fe9 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -179,6 +179,10 @@ static int ixgbevf_dev_xstats_get(struct rte_eth_dev
> *dev,
> struct rte_eth_xstats *xstats, unsigned n);
> static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev);
> static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev);
> +static int ixgbe_dev_xstats_names(__rte_unused struct rte_eth_dev *dev,
> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit);
> +static int ixgbevf_dev_xstats_names(__rte_unused struct rte_eth_dev *dev,
> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit);
> static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
>uint16_t queue_id,
>uint8_t stat_idx,
> @@ -466,6 +470,7 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = {
> .xstats_get = ixgbe_dev_xstats_get,
> .stats_reset = ixgbe_dev_stats_reset,
> .xstats_reset = ixgbe_dev_xstats_reset,
> + .xstats_names = ixgbe_dev_xstats_names,
> .queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set,
> .dev_infos_get= ixgbe_dev_info_get,
> .dev_supported_ptypes_get = ixgbe_dev_supported_ptypes_get,
> @@ -555,6 +560,7 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops =
> {
> .xstats_get = ixgbevf_dev_xstats_get,
> .stats_reset = ixgbevf_dev_stats_reset,
> .xstats_reset = ixgbevf_dev_stats_reset,
> + .xstats_names = ixgbevf_dev_xstats_names,
> .dev_close= ixgbevf_dev_close,
> .allmulticast_enable = ixgbevf_dev_allmulticast_enable,
> .allmulticast_disable = ixgbevf_dev_allmulticast_disable,
> @@ -2698,6 +2704,76 @@ ixgbe_xstats_calc_num(void) {
> (IXGBE_NB_TXQ_PRIO_STATS * 8);
> }
>
> +static int ixgbe_dev_xstats_names(__rte_unused struct rte_eth_dev *dev,
> + struct rte_eth_xstats_name *ptr_names, __rte_unused unsigned limit)
> +{
> + const unsigned cnt_stats = ixgbe_xstats_calc_num();
> + unsigned stat, i, count, offset;
> +
> + if (ptr_names != NULL) {
> + count = 0;
> + offset = 0;
> +
> + /* Note: limit >= cnt_stats checked upstream
> + * in rte_eth_xstats_names()
> + */
> +
> + /* Extended stats from ixgbe_hw_stats */
> + for (i = 0; i < IXGBE_NB_HW_STATS; i++) {
> + snprintf(ptr_names[count].name,
> + sizeof(ptr_names[count].name),
> + "%s",
> + rte_ixgbe_stats_strings[i].name);
> + count++;
> + offset += RTE_ETH_XSTATS_NAME_SIZE;
> + }
> +
> + /* RX Priority Stats */
> + for (stat = 0; stat < IXGBE_NB_RXQ_PRIO_STATS; stat++) {
> + for (i = 0; i < 8; i++) {
8 seems magical. Is there a constant somewhere that can be used?
> + snprintf(ptr_names[count].name,
> + sizeof(ptr_names[count].name),
> + "rx_priority%u_%s", i,
> + rte_ixgbe_rxq_strings[stat].name);
> + count++;
> + offset += RTE_ETH_XSTATS_NAME_SIZE;
> + }
> + }
> +
> + /* TX Priority Stats */
> + for (stat = 0; stat < IXGBE_NB_TXQ_PRIO_STATS; stat++) {
> + for (i = 0; i < 8; i++) {
Same magic number.
> + snprintf(ptr_names[count].name,
> + sizeof(ptr_names[count].name),
> + "tx_priority%u_%s", i,
> + rte_ixgbe_txq_strings[stat].name);
> + count++;
> + offset += RTE_ETH_XSTATS_NAME_SIZE;
> + }
> + }
> + /* FIXME: Debugging check */
Just a reminder to cleanup.
> + if (cnt_stats != count)
> + return -EIO;
> + }
> + return cnt_stats;
> +}
> +
> +static int ixgbevf_dev_xstats_names(__rte_unused struct rte_e
[dpdk-dev] [RFC PATCH v1 1/3] rte: change xstats to use integer keys
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
> Sent: Friday, April 15, 2016 10:44 AM
> To: dev at dpdk.org; Thomas Monjalon
> Subject: [dpdk-dev] [RFC PATCH v1 1/3] rte: change xstats to use integer
> keys
>
> Signed-off-by: Remy Horton
> ---
> lib/librte_ether/rte_ethdev.c | 87
> +++
> lib/librte_ether/rte_ethdev.h | 38 +++
> 2 files changed, 117 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a31018e..cdd0685 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1507,6 +1507,82 @@ rte_eth_stats_reset(uint8_t port_id)
> dev->data->rx_mbuf_alloc_failed = 0;
> }
>
> +static int
> +rte_eth_xstats_count(uint8_t port_id)
Thanks for adding this. I believe an overt API is much more clear.
> +{
> + struct rte_eth_dev *dev;
> + int count;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> + dev = &rte_eth_devices[port_id];
> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->xstats_names, -ENOTSUP);
> + count = (*dev->dev_ops->xstats_names)(dev, NULL, 0);
> + if (count >= 0) {
> + count += RTE_NB_STATS;
> + count += dev->data->nb_rx_queues * RTE_NB_RXQ_STATS;
> + count += dev->data->nb_tx_queues * RTE_NB_TXQ_STATS;
> + }
> + return count;
> +}
> +
> +int
> +rte_eth_xstats_names(uint8_t port_id, struct rte_eth_xstats_name
> *ptr_names,
> + unsigned limit)
> +{
> + struct rte_eth_dev *dev;
> + int cnt_used_entries;
> + int cnt_expected_entries;
> + uint32_t idx, id_queue;
> + int offset;
> +
> + cnt_expected_entries = rte_eth_xstats_count(port_id);
> + if (cnt_expected_entries < 0 || ptr_names == NULL)
> + return cnt_expected_entries;
I suggest we don't provide two ways to get the number of stats and
that users always call rte_eth_xstats_count().
Recommend returning -EINVAL if ptr_names is NULL.
> +
> + if ((int)limit < cnt_expected_entries)
> + return -ERANGE;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> + dev = &rte_eth_devices[port_id];
> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->xstats_names, -ENOTSUP);
I think this check is too restrictive. There are drivers that do not
provide device specific xstats today but the xstats API will still
return the per q stats.
Recommend skipping the device specific steps that follow.
> + cnt_used_entries = (*dev->dev_ops->xstats_names)(
> + dev, ptr_names, limit);
> +
> + if (cnt_used_entries < 0)
> + return cnt_used_entries;
> +
> + offset = cnt_used_entries * RTE_ETH_XSTATS_NAME_SIZE;
> + for (idx = 0; idx < RTE_NB_STATS; idx++) {
> + snprintf(ptr_names[cnt_used_entries].name,
> + sizeof(ptr_names[0].name),
> + "%s", rte_stats_strings[idx].name);
> + offset += RTE_ETH_XSTATS_NAME_SIZE;
> + cnt_used_entries++;
> + }
> + for (id_queue = 0; id_queue < dev->data->nb_rx_queues; id_queue++) {
> + for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) {
> + snprintf(ptr_names[cnt_used_entries].name,
> + sizeof(ptr_names[0].name),
> + "rx_q%u%s",
> + id_queue, rte_rxq_stats_strings[idx].name);
> + offset += RTE_ETH_XSTATS_NAME_SIZE;
> + cnt_used_entries++;
> + }
> +
> + }
> + for (id_queue = 0; id_queue < dev->data->nb_tx_queues; id_queue++) {
> + for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) {
> + snprintf(ptr_names[cnt_used_entries].name,
> + sizeof(ptr_names[0].name),
> + "tx_q%u%s",
> + id_queue, rte_txq_stats_strings[idx].name);
> + offset += RTE_ETH_XSTATS_NAME_SIZE;
> + cnt_used_entries++;
> + }
> + }
> + return cnt_used_entries;
> +}
> +
> /* retrieve ethdev extended statistics */
> int
> rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
> @@ -1551,8 +1627,7 @@ rte_eth_xstats_get(uint8_t port_id, struct
> rte_eth_xstats *xstats,
> stats_ptr = RTE_PTR_ADD(ð_stats,
> rte_stats_strings[i].offset);
> val = *stats_ptr;
> - snprintf(xstats[count].name, sizeof(xstats[count].name),
> - "%s", rte_stats_strings[i].name);
> + xstats[count].key = count + xcount;
Suggest setting adding:
xstats[count].name[0] = '\0'
until name is removed.
> xstats[count++].value = val;
> }
>
> @@ -1563,9 +1638,7 @@ rte_eth_xstats_get(uint8_t port_id, struct
> rte_eth_xstats *xstats,
>
[dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from xstats
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton > Sent: Friday, April 15, 2016 10:44 AM > To: dev at dpdk.org > Subject: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats > > The current extended ethernet statistics fetching involve doing several > string operations, which causes performance issues if there are lots of > statistics and/or network interfaces. This RFC patchset changes the API > for xstats to use integer identifiers instead of strings and implements > this new API for the ixgbe driver. Others drivers to follow. > > -- > > Since this will involve API & ABI breakage as previously advertised, > there are several design assumptions that need consideration: > > *) id-name & id-value pairs for both lookup and query > Permits out-of-order and non-contigious returning of names/ids/values, > even though expected implmentations would in practice return items in > sorted order by id. Is this sufficent/desirable future proofing? Idea > is to allow possibility of drivers returning partial statistics. I think the key is that the order of the stats must always be honored and if that's the case then an id isn't necessary. However, if others want an id certainly doesn't hurt. I don't see drivers autonomously providing a subset of stats and users can filter out stats they don't want to their presentation layers. > > *) Bulk name-id mapping lookup only > At the moment individual lookup is not supported, as this would impose > extra overheads on drivers. The assumption is that any end user would > fetch all this data once on startup and then cache the mappings. Agreed. Similarly there is no need to return a partial list of stats as the presentation layers can filter. > > *) Replacement or additional API > This patch replaces the current xstats API, but there is no inherant > reason beyond maintainability why this funtionality could not be in > addition rather than a replacement. What is consensus on this? I suggest 3 new functions are added: - get number of xstats - get xstats names - get xstats values This facilitates: - parallel development within the release without breaking current usage - possibility of removing rte_eth_xstats_get() in following release Thanks for moving this forward, Dave > > Comments welcome. > > Remy Horton (3): > rte: change xstats to use integer keys > drivers/net/ixgbe: change xstats to use integer keys > examples/ethtool: add xstats display command > > drivers/net/ixgbe/ixgbe_ethdev.c | 87 > +++ > examples/ethtool/ethtool-app/ethapp.c | 57 +++ > lib/librte_ether/rte_ethdev.c | 87 > +++ > lib/librte_ether/rte_ethdev.h | 38 +++ > 4 files changed, 252 insertions(+), 17 deletions(-) > > -- > 2.5.5
[dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from xstats
Happy Friday, > -Original Message- > From: Remy Horton [mailto:remy.horton at intel.com] > Sent: Friday, April 29, 2016 6:22 AM > To: David Harton (dharton) ; Tahhan, Maryam > ; dev at dpdk.org > Cc: Mcnamara, John ; Van Haaren, Harry > > Subject: Re: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats > > Morning, > > On 28/04/2016 16:58, David Harton (dharton) wrote: [...] > > > Maybe I misread the patch series or missed one but I don't see where > > stats can be obtained without copying strings? This is the real issue > > I raised originally. > > See http://dpdk.org/dev/patchwork/patch/12096/ where xstats[].key is used > to lookup string from ptr_names[].name - I didn't delete the name field > from rte_eth_xstats because doing so would cause a compile error with the > drivers I've not yet converted. Ok, this was one of my fundamental hang ups. I didn't see any gain because of the way the proposal is being introduced. I guess you are saying that not only will drivers be added in future patch series but that you also intend to continue modifying the external API as well. I will start a clean thread and reply/review the provided patches and forgo my work. Cheers, Dave > > > > > I did not add "get the count" because it wasn't provided in the > > current API and instead followed the convention but I do believe > > overtly getting the count it is the better approach. > > I didn't either for the same reason, but if the API is going to be broken, > I think it should be added. > > > ..Remy
[dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from xstats
> -Original Message- > From: Tahhan, Maryam [mailto:maryam.tahhan at intel.com] > Sent: Thursday, April 28, 2016 10:56 AM > To: David Harton (dharton) ; Horton, Remy > ; dev at dpdk.org > Cc: Mcnamara, John ; Van Haaren, Harry > > Subject: RE: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of David Harton > > (dharton) > > Sent: Wednesday, April 20, 2016 5:04 PM > > To: Horton, Remy ; dev at dpdk.org > > Subject: Re: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations > > from xstats > > > > > -Original Message- > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton > > > Sent: Friday, April 15, 2016 10:44 AM > > > To: dev at dpdk.org > > > Subject: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > > > xstats > > > > > > The current extended ethernet statistics fetching involve doing > > > several string operations, which causes performance issues if there > > > are lots of statistics and/or network interfaces. This RFC patchset > > > changes the API for xstats to use integer identifiers instead of > > > strings and implements this new API for the ixgbe driver. Others > > drivers to follow. > > > > > > -- > > > > > > Since this will involve API & ABI breakage as previously advertised, > > > there are several design assumptions that need consideration: > > > > > > *) id-name & id-value pairs for both lookup and query Permits > > > out-of-order and non-contigious returning of names/ids/values, even > > > though expected implmentations would in practice return items in > > > sorted order by id. Is this sufficent/desirable future proofing? > > > Idea is to allow possibility of drivers returning partial statistics. > > > > I believe forcing drivers to match to a common id-space will become > > burdensome. If the stats id-space isn't common then matching strings > > is probably just as sufficient as long as drivers don't add/remove > > stats ad hoc between the time the device is initialized and removed. > > I'm not aware of drivers adding/removing the stats ad hoc? The idea is to > have a common-id space otherwise it will be a free for all and we won't > have alignment across the drivers. I don't see it being any more > burdensome than having a common register naming across the board which is > what is there today. The advantage being that you don't have to pull the > strings every time. > > > > > > > > > *) Bulk name-id mapping lookup only > > > At the moment individual lookup is not supported, as this would > > > impose extra overheads on drivers. The assumption is that any end > > > user would fetch all this data once on startup and then cache the > mappings. > > > > I'm not sure I see the value of looking up a single stat from a user > > perspective. I can see where the drivers might say that some stats > > are less disruptive/etc but the user doesn't have that knowledge and > > wouldn't know how to take advantage. Usually all stats are grabbed > > multiple times and the changes noted during debug sessions. > > > > I believe Remy's change doesn't suggest/support individual lookup. It is > just a statement that we don't want to burden drivers with individual > stats lookups. > > > > > > > *) Replacement or additional API > > > This patch replaces the current xstats API, but there is no inherant > > > reason beyond maintainability why this funtionality could not be in > > > addition rather than a replacement. What is consensus on this? > > > > I came to the conclusion that replacing the existing API isn't > > necessary but rather extending it so backwards compatibility could be > > maintained during the previous discussions on this topic. However, if > > we want to go forward with cleaning up in order to reduce the support > > drivers provide I'm all for it. > > > > I still believe the API we develop should follow an "ethtool stats like" > > format as suggested earlier this year: > > > > extern int rte_eth_xstats_names_get(uint8_t port_id, > > struct rte_eth_xstats_name *names, unsigned n); extern int > > rte_eth_xstats_values_get(uint8_t port_id, > > uint64_t *values, unsigned n); > > > > Again, these could be provided alongside the existing API or replace it. > > I'm struggl
[dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from xstats
> -Original Message- > From: Mcnamara, John [mailto:john.mcnamara at intel.com] > Sent: Wednesday, April 20, 2016 12:50 PM > To: David Harton (dharton) ; Horton, Remy > ; dev at dpdk.org > Cc: Van Haaren, Harry > Subject: RE: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats > > > -Original Message- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of David Harton > > (dharton) > > Sent: Wednesday, April 20, 2016 5:04 PM > > To: Horton, Remy ; dev at dpdk.org > > Subject: Re: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations > > from xstats > > > > > > Again, I can provide the patches for the changes I've made that align > > with this proposed API. I just never got any feedback on it when > > requested previously. > > Hi David, > > Perhaps it would be good to submit your patches, also with an RFC tag, so > we can make a comparison. Will do. Timing is a little bad right now due to personal commitments. Can we wait a couple weeks? Thanks, Dave > > Thanks, > > John. > --
[dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from xstats
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton > Sent: Friday, April 15, 2016 10:44 AM > To: dev at dpdk.org > Subject: [dpdk-dev] [RFC PATCH v1 0/3] Remove string operations from > xstats > > The current extended ethernet statistics fetching involve doing several > string operations, which causes performance issues if there are lots of > statistics and/or network interfaces. This RFC patchset changes the API > for xstats to use integer identifiers instead of strings and implements > this new API for the ixgbe driver. Others drivers to follow. > > -- > > Since this will involve API & ABI breakage as previously advertised, there > are several design assumptions that need consideration: > > *) id-name & id-value pairs for both lookup and query Permits out-of-order > and non-contigious returning of names/ids/values, even though expected > implmentations would in practice return items in sorted order by id. Is > this sufficent/desirable future proofing? Idea is to allow possibility of > drivers returning partial statistics. I believe forcing drivers to match to a common id-space will become burdensome. If the stats id-space isn't common then matching strings is probably just as sufficient as long as drivers don't add/remove stats ad hoc between the time the device is initialized and removed. > > *) Bulk name-id mapping lookup only > At the moment individual lookup is not supported, as this would impose > extra overheads on drivers. The assumption is that any end user would > fetch all this data once on startup and then cache the mappings. I'm not sure I see the value of looking up a single stat from a user perspective. I can see where the drivers might say that some stats are less disruptive/etc but the user doesn't have that knowledge and wouldn't know how to take advantage. Usually all stats are grabbed multiple times and the changes noted during debug sessions. > > *) Replacement or additional API > This patch replaces the current xstats API, but there is no inherant > reason beyond maintainability why this funtionality could not be in > addition rather than a replacement. What is consensus on this? I came to the conclusion that replacing the existing API isn't necessary but rather extending it so backwards compatibility could be maintained during the previous discussions on this topic. However, if we want to go forward with cleaning up in order to reduce the support drivers provide I'm all for it. I still believe the API we develop should follow an "ethtool stats like" format as suggested earlier this year: extern int rte_eth_xstats_names_get(uint8_t port_id, struct rte_eth_xstats_name *names, unsigned n); extern int rte_eth_xstats_values_get(uint8_t port_id, uint64_t *values, unsigned n); Again, these could be provided alongside the existing API or replace it. I also like the idea you provided of a separate API to obtain the xstats count rather than deriving the count by calling one of the above functions with "dummy" values. Again, I can provide the patches for the changes I've made that align with this proposed API. I just never got any feedback on it when requested previously. Regards, Dave > > Comments welcome. > > Remy Horton (3): > rte: change xstats to use integer keys > drivers/net/ixgbe: change xstats to use integer keys > examples/ethtool: add xstats display command > > drivers/net/ixgbe/ixgbe_ethdev.c | 87 > +++ > examples/ethtool/ethtool-app/ethapp.c | 57 +++ > lib/librte_ether/rte_ethdev.c | 87 > +++ > lib/librte_ether/rte_ethdev.h | 38 +++ > 4 files changed, 252 insertions(+), 17 deletions(-) > > -- > 2.5.5
[dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Harry van Haaren > Sent: Tuesday, April 05, 2016 1:58 PM > To: dev at dpdk.org > Cc: maryam.tahhan at intel.com; Harry van Haaren intel.com> > Subject: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07 > > This patch adds a notice that the API for the xstats functionality will be > modified in the 16.07 release, with no backwards compatibility planned as > it would require code duplication in each PMD that supports xstats. > > Signed-off-by: Harry van Haaren > --- > doc/guides/rel_notes/deprecation.rst | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/doc/guides/rel_notes/deprecation.rst > b/doc/guides/rel_notes/deprecation.rst > index 98d5529..13c3a95 100644 > --- a/doc/guides/rel_notes/deprecation.rst > +++ b/doc/guides/rel_notes/deprecation.rst > @@ -54,3 +54,8 @@ Deprecation Notices >induce a modification of the rte_mempool structure, plus a >modification of the API of rte_mempool_obj_iter(), implying a breakage >of the ABI. > + > +* ABI change is planned for the xstats API and rte_eth_xstats struct, > +to > + facilitate updating to an API that allows retrieval of values without > +any > + string copies or parsing. No backwards compatibility is planned, as > +it would > + require code duplication in every PMD that supports xstats. > -- > 2.5.0 Acked-by: David Harton
[dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Wednesday, April 06, 2016 8:14 AM > To: Van Haaren, Harry > Cc: David Harton (dharton) ; dev at dpdk.org; Tahhan, > Maryam ; olivier.matz at 6wind.com > Subject: Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07 > > 2016-04-06 11:16, Van Haaren, Harry: > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > > > The issue we are going to fix is that currently PMDs copy strings > > > > when retrieving > > > statistics, which causes unnecessary overhead. The implementation is > > > not decided yet, but using an int->value mapping seems logical. > > > > > I am not sure performance is so much critical when retrieving > statistics. > > > > In the previous discussion David was concerned about performance > > impact of string copies, are those concerns still present David? > > > > > The extended stats can be infinitely extended. So a string > > > identifier seems a lot more natural. > > > > I'm not suggesting that the string identifier is removed totally. > > > > > I do not agree to add a new numeric identifier in the API each time > > > a driver wants to report a specific statistic for debugging purpose. > > > > And I agree - the ints are just an index to xstats arrays, no eth-dev > wide enums here. Yes, I abandoned the idea of a set of stats ids. I can see where registration will be problematic and cumbersome to driver developers. > > The proposal is to make the API more flexible, see example: > > http://thread.gmane.org/gmane.comp.networking.dpdk.devel/31728/focus=3 > > 2795 > > > > This more flexible API would allow other types of information about > > statistics be retrieved too. I have prototyped this. If there is interest/acceptance I can work on making an official patch to share back to the community. Using this method still gives the flexibility the current API desires while giving the user the control to only obtain the counters. This of course assumes that the counters per device are static but that seems a safe bet. > > OK I think I start to understand. > > > For now, the sent patch announces that the API/ABI may change, and we > > can discuss details of API as development starts. > > This should not be the normal process. > It is important to understand what should be the changes to decide of > announcing or not a deprecation. > In the case of the mempool reworks, the patch have been sent and discussed > on the mailing list. > Given the previous explanations (and knowing you did good job on stats), I > give my > Acked-by: Thomas Monjalon Thanks for considering this. Regards, Dave
[dpdk-dev] Future Direction for rte_eth_stats_get()
> From: Van Haaren, Harry [mailto:harry.van.haaren at intel.com]
>
> > From: David Harton
> > Subject: RE: [dpdk-dev] Future Direction for rte_eth_stats_get()
> >
> > Hi folks,
> >
> > I didn't see any follow up to this response.
>
> I think you may have missed one:
> http://dpdk.org/ml/archives/dev/2016-January/032211.html
Apologies Harry! I didn't see your original post because the IT gods had
decided your response was "Junk" mail and it didn't make it to my dev_dpdk.org
mail folder. :(
A colleague actually pointed me to this post separately today. I've made the
Junk mailer a little smarter now...hopefully.
>
> I'm looking at the enum thinking it will grow out of control.
> Have you thought about adding metadata for RX / TX, PF / VF?
Yes, after thinking about it more I think it could get crazy.
>
> If metadata info is added, it would make retrieving a set of statistics
> based on a certain mask much easier. Do you think this may be of use?
Actually, I put a fair bit of thought into things and then realized, why
re-invent the wheel?
Why not follow the ethtool stats model?
struct rte_eth_xstats_name {
char name[RTE_ETH_XSTATS_NAME_SIZE]; };
extern int rte_eth_xtats_count(uint8_t port_id, unsigned *count);
extern int rte_eth_xtats_strings(uint8_t port_id, unsigned count, struct
rte_eth_xtats_name *names);
extern int rte_eth_xtats_values(uint8_t port_id, unsigned count, uint64_t
*values);
The existing API could be left in-place and these could be added for folks that
don't want to grab the strings all the time.
The cons compared to providing an enum or extending struct rte_eth_stats are:
- you have to perform a query immediately after the device is attached
- doesn't require conformity...which has pros and cons
I'm actually testing the changes above if folks think this would be a
reasonable compromise I can patch them up.
I still feel the feedback myself and others gave about rte_eth_stats_get()
being closer to a standard MIB should get some consideration. Applications
that run with a number of different drivers/device types likely want to avoid
having to create "xstats mapping tables" every time a new device pops out just
so they can debug problems.
Thanks,
Dave
[dpdk-dev] Future Direction for rte_eth_stats_get()
Hi folks,
I didn't see any follow up to this response.
My original concern was rte_eth_stats_get() moving away from a more
conventional based definition (note, I believe Matthew Hall made an interesting
suggestion to follow a MIB based definition elsewhere). However, if modifying
that API is not desired then I'd really like to have some feedback about
extending the current xstats model.
Again, it is desired not to have to copy and/or parse strings for scalability
reasons but still maintain the "ABI flexibility" for which the xstats model was
geared towards.
Thanks,
Dave
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of David Harton (dharton)
> Sent: Friday, January 22, 2016 2:26 PM
> To: Van Haaren, Harry ; Thomas Monjalon
>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] Future Direction for rte_eth_stats_get()
>
> > > > Do you see a fundamental problem with parsing the strings to
> > > > retrieve values?
> > >
> > > I think parsing strings is expensive CPU wise
> >
> > Parsing the strings can be done once at startup, and the index of the
> > statistics in the array cached. When actually reading statistics,
> > access of the array of statistics at the previously cached index will
> > return the same statistic. It is not needed to do strcmp() per statistic
> per read.
>
> How is this order guaranteed through the API? The stat is currently
> identified by the string not by order returned. What if a driver returns
> more/less stats based on config that changes after boot?
>
> >
> >
> > > That's why I was wondering if a binary id could be generated instead.
> >
> > I've worked with such a system before, it dynamically registered
> > string-
> > >int mappings at runtime, and allowed "reverse" mapping them too. Its
> > workable, and I'm not opposed to it if the conclusion is that it
> > really is necessary, but I'm not sure about that.
> >
> >
> > > The API
> > > would be very similar to the current xstats API except it would pass
> > > id/value pairs instead of string/value pairs. This avoids string
> > > comparisons to figure out while still allowing flexibility between
> > drivers.
> >
> > Apart from a once-off scan at startup, xstats achieves this.
>
> Partially true. You may not need to perform strcmp() per read, although I
> don't believe the defined API guarantees that this would be safe (see
> above); but, you still have to perform a strcpy() of each stat whether the
> caller is interested in it or not.
>
> >
> >
> > > It would also 100% guarantee that any strings returned by "const
> > > char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent
> > > across devices.
> >
> > Yes - that is a nice feature that xstats (in its current form) doesn't
> > have.
> > But what price is there to pay for this?
> > We need to map an ID for each stat that exists.
> >
> > How and where will this mapping happen? Perhaps would you expand a
> > little on how you see this working?
>
> I wasn't thinking dynamic registration as that might be more than
> necessary. I can code up a detailed snippet that shares the idea if
> wanted but I think the following communicates the basic idea:
>
> enum rte_eth_stat_e {
> /* accurate desc #1 */
> RTE_ETH_STAT_1,
> /* accurate desc #2 */
> RTE_ETH_STAT_2,
> ...
> }
> struct rte_eth_id_stat {
> rte_eth_stat_e id;
> uin64_t value;
> }
>
> int rte_eth_id_stats_num(uint8_t port_id, uint32_t *num_stats);
> /* returns < 0 on error or the number of stats that could have been read
> (i.e. if userd */ int rte_eth_id_stats_get(uint8_t port_id, uint32_t
> num_stats, rte_eth_id_stat *id_stats); const char*
> rte_eth_id_stat_str(rte_eth_stat_e id);
>
> This allows a driver to return whatever stats that it supports in a
> consistent manner and also in a performance friendly way. In fact, the
> driver side would be identical to what they have today but instead of
> having arrays with "string stat name" they will have the rte_eth_stat_e.
>
> >
> > > I also think there is less chance for error if drivers assign their
> > > stats to ID versus constructing a string.
> >
> > Have a look in how the mapping is done in the xstats implementation
> > for ixgbe for example:
> > http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n540
> >
> > It's a pretty simple {"string stat name" -> offsetof( stuct hw,
> >
[dpdk-dev] Future Direction for rte_eth_stats_get()
> > > Do you see a fundamental problem with parsing the strings to
> > > retrieve values?
> >
> > I think parsing strings is expensive CPU wise
>
> Parsing the strings can be done once at startup, and the index of the
> statistics in the array cached. When actually reading statistics, access
> of the array of statistics at the previously cached index will return the
> same statistic. It is not needed to do strcmp() per statistic per read.
How is this order guaranteed through the API? The stat is currently identified
by the string not by order returned. What if a driver returns more/less stats
based on config that changes after boot?
>
>
> > That's why I was wondering if a binary id could be generated instead.
>
> I've worked with such a system before, it dynamically registered string-
> >int mappings at runtime, and allowed "reverse" mapping them too. Its
> workable, and I'm not opposed to it if the conclusion is that it really is
> necessary, but I'm not sure about that.
>
>
> > The API
> > would be very similar to the current xstats API except it would pass
> > id/value pairs instead of string/value pairs. This avoids string
> > comparisons to figure out while still allowing flexibility between
> drivers.
>
> Apart from a once-off scan at startup, xstats achieves this.
Partially true. You may not need to perform strcmp() per read, although I
don't believe the defined API guarantees that this would be safe (see above);
but, you still have to perform a strcpy() of each stat whether the caller is
interested in it or not.
>
>
> > It would also 100% guarantee that any strings returned by "const
> > char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent across
> > devices.
>
> Yes - that is a nice feature that xstats (in its current form) doesn't
> have.
> But what price is there to pay for this?
> We need to map an ID for each stat that exists.
>
> How and where will this mapping happen? Perhaps would you expand a little
> on how you see this working?
I wasn't thinking dynamic registration as that might be more than necessary. I
can code up a detailed snippet that shares the idea if wanted but I think the
following communicates the basic idea:
enum rte_eth_stat_e {
/* accurate desc #1 */
RTE_ETH_STAT_1,
/* accurate desc #2 */
RTE_ETH_STAT_2,
...
}
struct rte_eth_id_stat {
rte_eth_stat_e id;
uin64_t value;
}
int rte_eth_id_stats_num(uint8_t port_id, uint32_t *num_stats);
/* returns < 0 on error or the number of stats that could have been read (i.e.
if userd */
int rte_eth_id_stats_get(uint8_t port_id, uint32_t num_stats, rte_eth_id_stat
*id_stats);
const char* rte_eth_id_stat_str(rte_eth_stat_e id);
This allows a driver to return whatever stats that it supports in a consistent
manner and also in a performance friendly way. In fact, the driver side would
be identical to what they have today but instead of having arrays with "string
stat name" they will have the rte_eth_stat_e.
>
> > I also think there is less chance for error if drivers assign their
> > stats to ID versus constructing a string.
>
> Have a look in how the mapping is done in the xstats implementation for
> ixgbe for example:
> http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n540
>
> It's a pretty simple {"string stat name" -> offsetof( stuct hw,
> register_var_name )}
It's not how they add the strings but rather the format of the string itself
that is error prone.
IOTW, the "string stat name" is prone to not being formatted correctly or
consistently.
>
>
> > I haven't checked my application, but I wonder if the binary size
> > would actually be smaller if all the stats strings were centrally
> > located versus distributed across binaries (highly dependent on the
> linker and optimization level).
>
> Sounds like optimizing the wrong thing to me.
Wasn't trying to optimize image size as much as saying it's a side benefit due
to string consolidation.
>
>
> > > If you like I could code up a minimal sample-app that only pulls
> > > statistics, and you can show "interest" in various statistics for
> > > printing / monitoring?
> >
> > Appreciate the offer. I actually understand what's is available.
> > And, BTW, I apologize for being late to the game (looks like this was
> > discussed last summer) but I'm just now getting looped in and
> > following the mailer but I'm just wondering if something that is
> performance friendly for the user/application and flexible for the drivers
> is possible.
>
> We're all looking for the same thing - just different approaches that's
> all :)
>
>
> RE: Thomas asking about performance numbers:
> I can scrape together some raw tsc data on Monday and post to list, and we
> can discuss it more then.
I can do the same if desired. But, just to make sure we are discussing the
same issue:
1) call rte_eth_xtats_get()
This will result in many string copies and depending on the driver *many*
copies I don't want or
[dpdk-dev] Future Direction for rte_eth_stats_get()
> > xstats are driver agnostic and have a well-defined naming scheme. > > Indeed, described here: > http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html#extended- > statistics-api Thanks for sharing. I do think what is in the link is well thought out but it also may not match how the application would choose to represent that stat (capitalization, abbreviation, etc). > From: David Harton: > > For example, what if there was a kind of "stats registry" composed of > > ID and name. It would work similar to xtats except instead of > > advertising strings only the "get" API would return ID/count pairs. > > If the application wishes to use the DPDK provided string they can > > call another API to get the stat string via the ID. These IDs would > > be well-defined clearly explaining what the count represent. This way > > the strings for counts will be uniform across drivers and also make it > clear to the users what the counts truly represent and the application > could obtain stats from any driver in a driver agnostic manner. > > What you (David Harton) describe about a well-defined name, and clearly > explaining the value it is representing is what the goal was for xstats: a > human readable string, which can be easily parsed and a value retrieved. > > The "rules" of encoding a statistic as an xstats string are pretty simple, > and if any PMD breaks the rules, it should be considered broken and fixed. I understand it may be broken, but that doesn't help finding them and ensuring they aren't broken to begin with. What regression/automation is in place to ensure drivers aren't broken? > > Consistency is key, in this case consistency in the PMD xstats > implementations to make it useful for clients of the API. > > The big advantage xstats has over adding items to rte_eth_stats is that it > won't break ABI. 100% agree. I can see the intent. > > Do you see a fundamental problem with parsing the strings to retrieve > values? I think parsing strings is expensive CPU wise and again subject to error as devices are added. That's why I was wondering if a binary id could be generated instead. The binary id could also be used to look up the string name if the application wants it. The API would be very similar to the current xstats API except it would pass id/value pairs instead of string/value pairs. This avoids string comparisons to figure out while still allowing flexibility between drivers. It would also 100% guarantee that any strings returned by "const char* rte_eth_xstats_str_get(uint32_t stat_id)" are consistent across devices. I also think there is less chance for error if drivers assign their stats to ID versus constructing a string. I haven't checked my application, but I wonder if the binary size would actually be smaller if all the stats strings were centrally located versus distributed across binaries (highly dependent on the linker and optimization level). > > If you like I could code up a minimal sample-app that only pulls > statistics, and you can show "interest" in various statistics for printing > / monitoring? Appreciate the offer. I actually understand what's is available. And, BTW, I apologize for being late to the game (looks like this was discussed last summer) but I'm just now getting looped in and following the mailer but I'm just wondering if something that is performance friendly for the user/application and flexible for the drivers is possible. Thanks, Dave > > > Regards, -Harry
[dpdk-dev] Future Direction for rte_eth_stats_get()
Hi Maryam,
I'm not dictating they be re-added (although adding would be nice) but more
important I'm trying to express an application view point rather than a driver
view point.
I completely understand how a driver wants to be able to advertise all the
stats they want to advertise to help them debug their issues (i.e. xstats).
Yet, I'm very interested in DPDK providing a driver agnostic method of
advertising well-defined stats.
For example, what if there was a kind of "stats registry" composed of ID and
name. It would work similar to xtats except instead of advertising strings
only the "get" API would return ID/count pairs. If the application wishes to
use the DPDK provided string they can call another API to get the stat string
via the ID. These IDs would be well-defined clearly explaining what the count
represent. This way the strings for counts will be uniform across drivers and
also make it clear to the users what the counts truly represent and the
application could obtain stats from any driver in a driver agnostic manner.
Just an idea,
Dave
> -Original Message-
> From: Tahhan, Maryam [mailto:maryam.tahhan at intel.com]
> Sent: Friday, January 22, 2016 9:18 AM
> To: David Harton (dharton) ; dev at dpdk.org
> Cc: Olivier MATZ ; Van Haaren, Harry
>
> Subject: RE: Future Direction for rte_eth_stats_get()
>
> Hi David
>
> + Olivier and Harry as contributors and advisors in the past on the stats
> and stats API.
>
> So I pulled the rtnl_link_stats64 from http://lxr.free-
> electrons.com/source/include/uapi/linux/if_link.h#L41 and crossed them
> with http://dpdk.org/dev/patchwork/patch/5842/ and
> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h
>
> 40 /* The main device statistics structure */
> 41 struct rtnl_link_stats64 {
> 42 __u64 rx_packets; /* total packets received
> */ is in struct rte_eth_stats
> 43 __u64 tx_packets; /* total packets transmitted
> */ is in struct rte_eth_stats
> 44 __u64 rx_bytes; /* total bytes received
> */ is in struct rte_eth_stats
> 45 __u64 tx_bytes; /* total bytes transmitted
> */is in struct rte_eth_stats
> 46 __u64 rx_errors; /* bad packets received
> */ I'm working on a patch to distinguish between error
> and drops for struct rte_eth_stats at the moment only drops are reported
> through ierrors and oerrors and we are tied to those field name for
> backward compatibility.
> 47 __u64 tx_errors; /* packet transmit problems
> */Same comment as rx_errors.
> 48 __u64 rx_dropped; /* no space in linux buffers
> */ is in struct rte_eth_stats
> 49 __u64 tx_dropped; /* no space available in linux
> */ is in struct rte_eth_stats
> 50 __u64 multicast; /* multicast packets received
> */was deprecated in struct rte_eth_stats - but we can
> remove the notice and expose again from drivers we modified
> 51 __u64 collisions; Not in struct rte_eth_stats - not
> sure it's in xstats either...
> 52
> 53 /* detailed rx_errors: */
> 54 __u64 rx_length_errors;
> was deprecated in struct rte_eth_stats, is in xstats - but we can
> remove the notice and expose again from drivers we modified
> 55 __u64 rx_over_errors; /* receiver ring buff overflow
> */ was never exposed to struct rte_eth_stats - but is in xstats.
> 56 __u64 rx_crc_errors; /* recved pkt with crc error
> */ was deprecated in struct rte_eth_stats, is in xstats - but
> we can remove the notice and expose again from drivers we modified
> 57 __u64 rx_frame_errors;/* recv'd frame alignment
> error */ was never exposed to struct rte_eth_stats - but is in
> xstats.
> 58 __u64 rx_fifo_errors; /* recv'r fifo overrun
> */was never in struct rte_eth_stats. Not exposed by
> all drivers
> 59 __u64 rx_missed_errors; /* receiver missed packet
> */was deprecated in struct rte_eth_stats, is in xstats - but we
> can remove the notice
> 60
> 61 /* detailed tx_errors */
> 62 __u64 tx_aborted_errors;
> was never in struct rte_eth_stats. Not exposed by all drivers
> 63 __u64 tx_carrier_errors;
> was never in struct rte_eth_stats. Not exposed by all drivers
> 64 __u64 tx_fifo_errors;
> was never in struct rte_eth_stats. Not exposed by all drivers
> 65 __u64 tx_heartbeat_errors;
> was never i
[dpdk-dev] Future Direction for rte_eth_stats_get()
Hi Maryam, Thanks for the pointer. I'll review the convo's. Consider I have an application that uses many(all?) of the DPDK drivers and their netmap counterparts depending on configuration. The user interface provides a set of I/O stats to help debug I/O issues and that set is the same regardless of driver type. The set of stats provided matches what linux provides today since netmap existed before dpdk. What I want to avoid is having an application that is driver independent having to become driver dependent interpreting a bunch of strings (from xstats) or worse the layer running at the data plane core that is advertising the API needed by the application parsing those strings because the application cannot change the upper layer. What if instead of passing strings and values a set of stat ids and value are returned. At least this way the application can remain driver agnostic versus having to parse a free form string that likely isn't the same across driver types. Also, why wouldn't rte_eth_stats_get() align with linux which is the defacto standard? I understand that not every driver may not support every stat but that's ok. Just return 0 (pause frames, crc errors, etc). It's not like the list of linux stats is ever growing or advertising an absurd number of stats that aren't applicable to all drivers. Thanks again, Dave > -Original Message- > From: Tahhan, Maryam [mailto:maryam.tahhan at intel.com] > Sent: Friday, January 22, 2016 6:08 AM > To: David Harton (dharton) ; dev at dpdk.org > Subject: RE: Future Direction for rte_eth_stats_get() > > Hi David > Some of the stats were HW specific rather than generic stats that should > be exposed through rte_eth_stats and were migrated to the xstats API. > http://dpdk.org/ml/archives/dev/2015-June/019915.html. The naming was also > not generic enough to cover some of the drivers and in some cases what was > exposed was only a partial view of the relevant stats. > > They were marked as deprecated to deter people from using them in the > future, but haven't been removed from all the driver implementations yet. > The Registers that remain undeprecated are those considered to be generic. > > Which registers are you particularly interested in that have been > deprecated? Can you elaborate on what you mean by " scenarios where a > static view is expected " > > Thanks in advance. > > Best Regards, > Maryam > > > > -Original Message- > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of David Harton > > (dharton) > > Sent: Wednesday, January 20, 2016 5:19 PM > > To: dev at dpdk.org > > Subject: [dpdk-dev] Future Direction for rte_eth_stats_get() > > > > I see that some of the rte_eth_stats have been marked deprecated in 2.2 > > that are returned by rte_eth_stats_get(). Applications that utilize any > > number of device types rely on functions like this one to debug I/O > > issues. > > > > Is there a reason the stats have been deprecated? Why not keep the > > stats in line with the standard linux practices such as > rtnl_link_stats64? > > > > Note, using rte_eth_xstats_get() does not help for this particular > scenario > > because a common binary API is needed to communicate through > > various layers and also provide a consistent view/meaning to users. The > > xstats is excellent for debugging device specific scenarios but can't > help > > in scenarios where a static view is expected. > > > > Thanks, > > Dave
[dpdk-dev] Future Direction for rte_eth_stats_get()
I see that some of the rte_eth_stats have been marked deprecated in 2.2 that are returned by rte_eth_stats_get(). Applications that utilize any number of device types rely on functions like this one to debug I/O issues. Is there a reason the stats have been deprecated? Why not keep the stats in line with the standard linux practices such as rtnl_link_stats64? Note, using rte_eth_xstats_get() does not help for this particular scenario because a common binary API is needed to communicate through various layers and also provide a consistent view/meaning to users. The xstats is excellent for debugging device specific scenarios but can't help in scenarios where a static view is expected. Thanks, Dave
[dpdk-dev] [PATCH v7 0/4] User-space Ethtool
Acked-by: David Harton > -Original Message- > From: Liang-Min Larry Wang [mailto:liang-min.wang at intel.com] > Sent: Wednesday, June 17, 2015 6:22 PM > To: dev at dpdk.org > Cc: Andrew Harvey (agh); David Harton (dharton); Roger Melton (rmelton); > Liang-Min Larry Wang > Subject: [PATCH v7 0/4] User-space Ethtool > > This implementation is designed to provide a familar interface for > applications that rely on kernel-space driver to support ethtool_op and > net_device_op for device management. The initial implementation focuses on > ops that can be implemented through existing netdev APIs. More ops will be > supported in latter release. > > v7 change: > - Remove rte_eth_dev_get_ringparam implementation > v6 change: > - Rebase to match new changes over librte_ether > v5 change: > - Change API name from 'leng' to 'length' > - Remove unused data structure rte_dev_vf_info > - Remove placeholder API rte_eth_dev_set_ringparam > - Clean up set_mac_addr implementation > v4 change: > - Add rte_eth_xxx apis and respective ops over igb and ixgbe > to support ethtool and net device alike ops > - Add an example to demonstrate the use of ethtool library > v3 change: > - Fix a build issue > v2 change: > - Implement rte_eth_dev_default_mac_addr_set through dev_ops::mac_addr_set > so it would support NIC devices other than ixgbe and igb > > Liang-Min Larry Wang (4): > ethdev: add apis to support access device info > ixgbe: add ops to support ethtool ops > igb: add ops to support ethtool ops > examples: new example: l2fwd-ethtool > > drivers/net/e1000/igb_ethdev.c | 186 > drivers/net/e1000/igb_regs.h | 217 + > drivers/net/ixgbe/ixgbe_ethdev.c | 183 > drivers/net/ixgbe/ixgbe_regs.h | 357 > examples/l2fwd-ethtool/Makefile | 55 ++ > examples/l2fwd-ethtool/l2fwd-app/Makefile| 58 ++ > examples/l2fwd-ethtool/l2fwd-app/main.c | 1030 > ++ > examples/l2fwd-ethtool/l2fwd-app/netdev_api.h| 781 > examples/l2fwd-ethtool/l2fwd-app/shared_fifo.h | 151 > examples/l2fwd-ethtool/lib/Makefile | 55 ++ > examples/l2fwd-ethtool/lib/rte_ethtool.c | 301 +++ > examples/l2fwd-ethtool/lib/rte_ethtool.h | 378 > examples/l2fwd-ethtool/nic-control/Makefile | 55 ++ > examples/l2fwd-ethtool/nic-control/nic_control.c | 412 + > lib/librte_ether/Makefile|1 + > lib/librte_ether/rte_eth_dev_info.h | 57 ++ > lib/librte_ether/rte_ethdev.c| 115 +++ > lib/librte_ether/rte_ethdev.h| 117 +++ > lib/librte_ether/rte_ether_version.map |6 + > 19 files changed, 4515 insertions(+) > create mode 100644 drivers/net/e1000/igb_regs.h create mode 100644 > drivers/net/ixgbe/ixgbe_regs.h create mode 100644 examples/l2fwd- > ethtool/Makefile create mode 100644 examples/l2fwd-ethtool/l2fwd- > app/Makefile > create mode 100644 examples/l2fwd-ethtool/l2fwd-app/main.c > create mode 100644 examples/l2fwd-ethtool/l2fwd-app/netdev_api.h > create mode 100644 examples/l2fwd-ethtool/l2fwd-app/shared_fifo.h > create mode 100644 examples/l2fwd-ethtool/lib/Makefile > create mode 100644 examples/l2fwd-ethtool/lib/rte_ethtool.c > create mode 100644 examples/l2fwd-ethtool/lib/rte_ethtool.h > create mode 100644 examples/l2fwd-ethtool/nic-control/Makefile > create mode 100644 examples/l2fwd-ethtool/nic-control/nic_control.c > create mode 100644 lib/librte_ether/rte_eth_dev_info.h > > -- > 2.1.4
[dpdk-dev] [PATCHv2 0/5] ethdev: add new API to retrieve RX/TX queue information
Acked-by: David Harton > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Konstantin Ananyev > Sent: Thursday, June 18, 2015 9:19 AM > To: dev at dpdk.org > Subject: [dpdk-dev] [PATCHv2 0/5] ethdev: add new API to retrieve RX/TX > queue information > > Add the ability for the upper layer to query RX/TX queue information. > Right now supported for: > ixgbe, i40e, e1000 PMDs. > > Konstantin Ananyev (5): > ethdev: add new API to retrieve RX/TX queue information > i40e: add support for eth_(rxq|txq)_info_get > ixgbe: add support for eth_(rxq|txq)_info_get > e1000: add support for eth_(rxq|txq)_info_get > testpmd: add new command to display RX/TX queue information > > app/test-pmd/cmdline.c | 48 + > app/test-pmd/config.c| 67 ++ > app/test-pmd/testpmd.h | 2 ++ > drivers/net/e1000/e1000_ethdev.h | 12 +++ > drivers/net/e1000/em_ethdev.c| 2 ++ > drivers/net/e1000/em_rxtx.c | 38 > drivers/net/e1000/igb_ethdev.c | 4 +++ > drivers/net/e1000/igb_rxtx.c | 36 +++ > drivers/net/i40e/i40e_ethdev.c | 2 ++ > drivers/net/i40e/i40e_ethdev.h | 5 +++ > drivers/net/i40e/i40e_rxtx.c | 42 ++ > drivers/net/ixgbe/ixgbe_ethdev.c | 4 +++ > drivers/net/ixgbe/ixgbe_ethdev.h | 6 > drivers/net/ixgbe/ixgbe_rxtx.c | 42 ++ > lib/librte_ether/rte_ethdev.c| 54 > lib/librte_ether/rte_ethdev.h| 77 > +++- > 16 files changed, 440 insertions(+), 1 deletion(-) > > -- > 1.8.5.3
[dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin > Sent: Monday, June 15, 2015 9:46 AM > To: Wang, Liang-min; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access > device info > > > > > -Original Message- > > From: Wang, Liang-min > > Sent: Monday, June 15, 2015 2:26 PM > > To: Ananyev, Konstantin; dev at dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > access device info > > > > > > > > > -Original Message- > > > From: Ananyev, Konstantin > > > Sent: Friday, June 12, 2015 8:31 AM > > > To: Wang, Liang-min; dev at dpdk.org > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > access device info > > > > > > > > > > > > > -Original Message- > > > > From: Wang, Liang-min > > > > Sent: Thursday, June 11, 2015 10:51 PM > > > > To: Ananyev, Konstantin; dev at dpdk.org > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > > access device info > > > > > > > > > > > > > > > > > -Original Message- > > > > > From: Ananyev, Konstantin > > > > > Sent: Thursday, June 11, 2015 9:07 AM > > > > > To: Wang, Liang-min; dev at dpdk.org > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > support access device info > > > > > > > > > > > > > > > > > > > > > -Original Message- > > > > > > From: Wang, Liang-min > > > > > > Sent: Thursday, June 11, 2015 1:58 PM > > > > > > To: Ananyev, Konstantin; dev at dpdk.org > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > support access device info > > > > > > > > > > > > Hi Konstantin, > > > > > > > > > > > > > -Original Message- > > > > > > > From: Ananyev, Konstantin > > > > > > > Sent: Thursday, June 11, 2015 8:26 AM > > > > > > > To: Wang, Liang-min; dev at dpdk.org > > > > > > > Cc: Wang, Liang-min > > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > support access device info > > > > > > > > > > > > > > Hi Larry, > > > > > > > > > > > > > > > -Original Message- > > > > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of > > > > > > > > Liang-Min Larry Wang > > > > > > > > Sent: Wednesday, June 10, 2015 4:10 PM > > > > > > > > To: dev at dpdk.org > > > > > > > > Cc: Wang, Liang-min > > > > > > > > Subject: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > > support access device info > > > > > > > > > > > > > > > > add new apis: > > > > > > > > - rte_eth_dev_default_mac_addr_set > > > > > > > > - rte_eth_dev_reg_leng > > > > > > > > - rte_eth_dev_reg_info > > > > > > > > - rte_eth_dev_eeprom_leng > > > > > > > > - rte_eth_dev_get_eeprom > > > > > > > > - rte_eth_dev_set_eeprom > > > > > > > > - rte_eth_dev_get_ringparam > > > > > > > > - rte_eth_dev_set_ringparam > > > > > > > > > > > > > > > > to enable reading device parameters (mac-addr, register, > > > > > > > > eeprom, > > > > > > > > ring) based upon ethtool alike data parameter specification. > > > > > > > > > > > > > > > > Signed-off-by: Liang-Min Larry Wang > > > > > > > > > > > > > > > > --- > > > > > > > > lib/librte_ether/Makefile | 1 + > > > > > > > > lib/librte_ether/rte_eth_dev_info.h| 80 > + > > > > > > > > lib/librte_ether/rte_ethdev.c | 159 > > > > > > > + > > > > > > > > lib/librte_ether/rte_ethdev.h | 158 > > > > > > > > > > > > > > > lib/librte_ether/rte_ether_version.map | 8 ++ > > > > > > > > 5 files changed, 406 insertions(+) create mode 100644 > > > > > > > > lib/librte_ether/rte_eth_dev_info.h > > > > > > > > > > > > > > > > diff --git a/lib/librte_ether/Makefile > > > > > > > > b/lib/librte_ether/Makefile index c0e5768..05209e9 100644 > > > > > > > > --- a/lib/librte_ether/Makefile > > > > > > > > +++ b/lib/librte_ether/Makefile > > > > > > > > @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c > > > > > > > > SYMLINK-y-include += rte_ether.h SYMLINK-y-include += > > > > > > > > rte_ethdev.h SYMLINK-y-include > > > > > > > > += rte_eth_ctrl.h > > > > > > > > +SYMLINK-y-include += rte_eth_dev_info.h > > > > > > > > > > > > > > > > # this lib depends upon: > > > > > > > > DEPDIRS-y += lib/librte_eal lib/librte_mempool > > > > > > > > lib/librte_ring lib/librte_mbuf diff --git > > > > > > > > a/lib/librte_ether/rte_eth_dev_info.h > > > > > > > > b/lib/librte_ether/rte_eth_dev_info.h > > > > > > > > new file mode 100644 > > > > > > > > index 000..002c4b5 > > > > > > > > --- /dev/null > > > > > > > > +++ b/lib/librte_ether/rte_eth_dev_info.h > > > > > > > > @@ -0,0 +1,80 @@ > > > > > > > > +/*- > > > > > > > > + * BSD LICENSE > > > > > > > > + * > > > > > > > > + * Copyright(c) 2015 Intel Corporation. All rights > reserved. > > > > > > > > + * All rights reserved. > > > > > > > > + * > > > > > > > > + * Redistribution and use i
[dpdk-dev] [PATCH v6 1/4] ethdev: add apis to support access device info
> -Original Message-
> From: Liang-Min Larry Wang [mailto:liang-min.wang at intel.com]
> Sent: Friday, June 12, 2015 6:03 PM
> To: dev at dpdk.org
> Cc: Andrew Harvey (agh); David Harton (dharton); Roger Melton (rmelton);
> Liang-Min Larry Wang
> Subject: [PATCH v6 1/4] ethdev: add apis to support access device info
>
> add new apis:
> - rte_eth_dev_default_mac_addr_set
> - rte_eth_dev_reg_length
> - rte_eth_dev_reg_info
> - rte_eth_dev_eeprom_length
> - rte_eth_dev_get_eeprom
> - rte_eth_dev_set_eeprom
> - rte_eth_dev_get_ringparam
>
> to enable reading device parameters (mac-addr, register,
> eeprom, ring) based upon ethtool alike
> data parameter specification.
>
> Signed-off-by: Liang-Min Larry Wang
> ---
> lib/librte_ether/Makefile | 1 +
> lib/librte_ether/rte_eth_dev_info.h| 67 +++
> lib/librte_ether/rte_ethdev.c | 134
> ++
> lib/librte_ether/rte_ethdev.h | 144
> +
> lib/librte_ether/rte_ether_version.map | 7 ++
> 5 files changed, 353 insertions(+)
> create mode 100644 lib/librte_ether/rte_eth_dev_info.h
>
> diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
> index c0e5768..05209e9 100644
> --- a/lib/librte_ether/Makefile
> +++ b/lib/librte_ether/Makefile
> @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c
> SYMLINK-y-include += rte_ether.h
> SYMLINK-y-include += rte_ethdev.h
> SYMLINK-y-include += rte_eth_ctrl.h
> +SYMLINK-y-include += rte_eth_dev_info.h
>
> # this lib depends upon:
> DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring
> lib/librte_mbuf
> diff --git a/lib/librte_ether/rte_eth_dev_info.h
> b/lib/librte_ether/rte_eth_dev_info.h
> new file mode 100644
> index 000..1cf2c23
> --- /dev/null
> +++ b/lib/librte_ether/rte_eth_dev_info.h
> @@ -0,0 +1,67 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright(c) 2015 Intel Corporation. All rights reserved.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Intel Corporation nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_ETH_DEV_INFO_H_
> +#define _RTE_ETH_DEV_INFO_H_
> +
> +/*
> + * Placeholder for accessing device registers
> + */
> +struct rte_dev_reg_info {
> + void *buf; /**< Buffer for register */
> + uint32_t offset; /**< Offset for 1st register to fetch */
> + uint32_t leng; /**< Number of registers to fetch */
> + uint32_t version; /**< Device version */
> +};
> +
> +/*
> + * Placeholder for accessing device eeprom
> + */
> +struct rte_dev_eeprom_info {
> + void *buf; /**< Buffer for eeprom */
> + uint32_t offset; /**< Offset for 1st eeprom location to access */
> + uint32_t leng; /**< Length of eeprom region to access */
> + uint32_t magic; /**< Device ID */
> +};
> +
> +/*
> + * Placeholder for accessing device ring parameters
> + */
> +struct rte_dev_ring_info {
> + uint32_t rx_pending; /**< Number of outstanding Rx ring */
> + uint32_t tx_pending; /**< Number of
[dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info
Sorry for spam and providing feedback here again.
> -Original Message-
> From: Liang-Min Larry Wang [mailto:liang-min.wang at intel.com]
> Sent: Wednesday, June 10, 2015 11:10 AM
> To: dev at dpdk.org
> Cc: Andrew Harvey (agh); Roger Melton (rmelton); David Harton (dharton);
> Liang-Min Larry Wang
> Subject: [PATCH v4 1/4] ethdev: add apis to support access device info
>
> add new apis:
> - rte_eth_dev_default_mac_addr_set
> - rte_eth_dev_reg_leng
> - rte_eth_dev_reg_info
> - rte_eth_dev_eeprom_leng
I know this is a bit of a nit but please consider changing "leng" to
"length".
Some places already use "length" in the diffs below so using length would be
consistent.
> - rte_eth_dev_get_eeprom
> - rte_eth_dev_set_eeprom
> - rte_eth_dev_get_ringparam
> - rte_eth_dev_set_ringparam
>
> to enable reading device parameters (mac-addr, register, eeprom, ring)
> based upon ethtool alike data parameter specification.
>
> Signed-off-by: Liang-Min Larry Wang
> ---
> lib/librte_ether/Makefile | 1 +
> lib/librte_ether/rte_eth_dev_info.h| 80 +
> lib/librte_ether/rte_ethdev.c | 159
> +
> lib/librte_ether/rte_ethdev.h | 158
>
> lib/librte_ether/rte_ether_version.map | 8 ++
> 5 files changed, 406 insertions(+)
> create mode 100644 lib/librte_ether/rte_eth_dev_info.h
>
> diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile index
> c0e5768..05209e9 100644
> --- a/lib/librte_ether/Makefile
> +++ b/lib/librte_ether/Makefile
> @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c SYMLINK-y-include += rte_ether.h
> SYMLINK-y-include += rte_ethdev.h SYMLINK-y-include += rte_eth_ctrl.h
> +SYMLINK-y-include += rte_eth_dev_info.h
>
> # this lib depends upon:
> DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring
> lib/librte_mbuf diff --git a/lib/librte_ether/rte_eth_dev_info.h
> b/lib/librte_ether/rte_eth_dev_info.h
> new file mode 100644
> index 000..002c4b5
> --- /dev/null
> +++ b/lib/librte_ether/rte_eth_dev_info.h
> @@ -0,0 +1,80 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright(c) 2015 Intel Corporation. All rights reserved.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above
> copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Intel Corporation nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_ETH_DEV_INFO_H_
> +#define _RTE_ETH_DEV_INFO_H_
> +
> +
> +/*
> + * Placeholder for accessing device registers */ struct
> +rte_dev_reg_info {
> + void *buf; /**< Buffer for register */
> + uint32_t offset; /**< Offset for 1st register to fetch */
> + uint32_t leng; /**< Number of registers to fetch */
> + uint32_t version; /**< Device version */ };
> +
> +/*
> + * Placeholder for accessing device eeprom */ struct
> +rte_dev_eeprom_info {
> + void *buf; /**< Buffer for eeprom */
> + uint32_t offset; /**< Offset for 1st eeprom location to access */
> + uint32_t leng; /**< Length of eeprom region to access */
> + uint32_t magic; /**<
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
Hi Thomas, I think Larry explains things pretty clearly below. A new application facing API is bring provided that avoids reusing kernel specific types and structs or dipping into the kernel itself. It also clearly separates the user space API from driver related functions/types. We do want to limit dipping into the kernel for performance reasons. I hope this helps, Dave > -Original Message- > From: Wang, Liang-min [mailto:liang-min.wang at intel.com] > Sent: Tuesday, June 02, 2015 4:56 PM > To: Thomas Monjalon > Cc: dev at dpdk.org; David Harton (dharton) > Subject: RE: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide > ethtool-alike APIs > > >I have the feeling we are not progressing in this discussion. > >Please bring new explanations or I'll give up. > >David Harton already acked it so maybe he could explain why it is useful. > > > >Comments below > > > >2015-06-02 17:06, Wang, Liang-min: > >> >2015-06-02 15:47, Wang, Liang-min: > >> >> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > >> >> > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() > >> >> > >>to > >> >> > >>rte_ethtool_net_change_mtu() will help anyone. > >> >> >> > >> >> >> As described, this interface is designed to provide API closely > to kernel space ethtool ops and net_device_op. > >> >> > >> >> >But the application still needs to adapt the code to call rte_* > functions. > >> >> >So changing to rte_ethtool_net_change_mtu is equivalent to change > to the existing rte_eth_dev_set_mtu. I don't see the benefit. > >> >> > >> >> The benefit is single interface for users to access. Instead of > looking into two different interfaces (ethtool, ether). > >> > > >> >Sorry it doesn't help to understand. > >> >Today, there is an ethdev API. Why adding an ethtool-like API would > help? > >> > >> Need to understand your specific concern. Do you oppose introducing new > API to support ethtool ops and net_device_ops? > > > >They are not ethtool/netdev ops but fake ones. > >In linux: > > int dev_set_mtu(struct net_device *dev, int new_mtu) In dpdk: > > int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu); So yes, > I'm opposed to add an API which is neither ethdev, neither ethtool. > >But I may be missing an obvious justification. > > So, the design purposely stays away from struct net_device to avoid > carrying kernel states. We consciously choose port in place of net_device. > The kni already provides ethtool for kernel space code, this interface is > designed for user-space application (fast-path comparing to kni). > > >> Or your concern is on the implementation. If that's latter. > >> Do you oppose adding a new library to implement ethtool ops and > net_device_ops? > > > >Already answered above > >> > >> If so, are you satisfied with my previous answer on avoiding > polluting ethdev APIs? > >> If not, do you suggest integrating ethtool APIs into ethdev > API? > > > >No, it's better as a separate library. > > > >> If not, is your concern on the implementation of common > functionality between ethtool and ethdev APIs? > >> If so, as explained, it is designed to provide a unified > >> interface to assist users to migrate from kernel > >> ethtool/net-device-op API to DPDK > > > >Do you mean it would help migrating some code from kernel space to dpdk? > >How it would help since the functions and data are different from the > kernel ones? > > For application that was designed based upon kernel-space ethtool-op and > net-device-op, the user-space APIs provide a path for quick integration. > > >> BTW, as my reply to Steve's comment, more ops are on their way. This > patch is to open up the interface. > > > >Currently they are only some one-liners plus ethtool_drvinfo formatting > so there is no real benefit. > >Why not wait to have more ops implemented? > Due to amount of code change, I was advised to put into separate release > and started with APIs that only requires minor changes on ethdev. > The rest of API requires changes on NIC pmd driver to support ops that are > not currently supported.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
Acked-by: David Harton (dharton) > -Original Message- > From: Liang-Min Larry Wang [mailto:liang-min.wang at intel.com] > Sent: Friday, May 29, 2015 8:38 PM > To: dev at dpdk.org > Cc: bruce.richardson at intel.com; konstantin.ananyev at intel.com; David > Harton > (dharton); Andrew Harvey (agh); Liang-Min Larry Wang > Subject: [PATCH 2/2] ethtool: add new library to provide ethtool-alike > APIs > > adding a new library based upon ethdev APIs to provide API's that bear > the same functionality as ethtool_ops (linux/ethtool.h) and net_device_ops > (linux/netdevice.h). > > Signed-off-by: Liang-Min Larry Wang > --- > MAINTAINERS| 4 + > config/common_linuxapp | 5 + > lib/Makefile | 1 + > lib/librte_ethtool/Makefile| 56 +++ > lib/librte_ethtool/rte_ethtool.c | 155 + > lib/librte_ethtool/rte_ethtool.h | 257 > + > lib/librte_ethtool/rte_ethtool_version.map | 18 ++ > mk/rte.app.mk | 1 + > 8 files changed, 497 insertions(+) > create mode 100644 lib/librte_ethtool/Makefile > create mode 100644 lib/librte_ethtool/rte_ethtool.c > create mode 100644 lib/librte_ethtool/rte_ethtool.h > create mode 100644 lib/librte_ethtool/rte_ethtool_version.map > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9362c19..b8b481f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -186,6 +186,10 @@ M: Thomas Monjalon > F: lib/librte_ether/ > F: scripts/test-null.sh > > +Ethtool API > +M: Liang-Min Larry Wang > +F: lib/librte_ethtool/ > + > > Drivers > --- > diff --git a/config/common_linuxapp b/config/common_linuxapp > index 0078dc9..f5759fd 100644 > --- a/config/common_linuxapp > +++ b/config/common_linuxapp > @@ -129,6 +129,11 @@ CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=y > CONFIG_RTE_LIBRTE_KVARGS=y > > # > +# Compile user-space ethtool library > +# > +CONFIG_RTE_LIBRTE_ETHTOOL=y > + > +# > # Compile generic ethernet library > # > CONFIG_RTE_LIBRTE_ETHER=y > diff --git a/lib/Makefile b/lib/Makefile > index 5f480f9..a6c7375 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -41,6 +41,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_TIMER) += librte_timer > DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile > DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline > DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether > +DIRS-$(CONFIG_RTE_LIBRTE_ETHTOOL) += librte_ethtool > DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += librte_vhost > DIRS-$(CONFIG_RTE_LIBRTE_HASH) += librte_hash > DIRS-$(CONFIG_RTE_LIBRTE_LPM) += librte_lpm > diff --git a/lib/librte_ethtool/Makefile b/lib/librte_ethtool/Makefile > new file mode 100644 > index 000..1d981f6 > --- /dev/null > +++ b/lib/librte_ethtool/Makefile > @@ -0,0 +1,56 @@ > +# BSD LICENSE > +# > +# Copyright(c) 2010-2015 Intel Corporation. All rights reserved. > +# All rights reserved. > +# > +# Redistribution and use in source and binary forms, with or without > +# modification, are permitted provided that the following conditions > +# are met: > +# > +# * Redistributions of source code must retain the above copyright > +# notice, this list of conditions and the following disclaimer. > +# * Redistributions in binary form must reproduce the above copyright > +# notice, this list of conditions and the following disclaimer in > +# the documentation and/or other materials provided with the > +# distribution. > +# * Neither the name of Intel Corporation nor the names of its > +# contributors may be used to endorse or promote products derived > +# from this software without specific prior written permission. > +# > +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + > +include $(RTE_SDK)/mk/rte.vars.
