Re: [PATCH v5 14/40] net/enic: check RSS hash algorithms

[John Daley (johndale)]

> also 'enicpmd_dev_configure()' looks like can be updated.
Where would it be updated? With the patch, the check will be done at the bottom 
of the function in call to enic_init_rss_nic_cfg -> enic_set_niccfg



From: Ferruh Yigit 
Date: Wednesday, October 11, 2023 at 10:32 AM
To: Jie Hai , dev@dpdk.org , John Daley 
(johndale) , Hyong Youb Kim (hyonkim) 
Cc: lihuis...@huawei.com , fengcheng...@huawei.com 
, liudongdo...@huawei.com 
Subject: Re: [PATCH v5 14/40] net/enic: check RSS hash algorithms
On 10/11/2023 10:27 AM, Jie Hai wrote:
> A new field 'algorithm' has been added to rss_conf, check it
> in case of ignoring unsupported values.
>
> Signed-off-by: Jie Hai 
> ---
>  drivers/net/enic/enic_ethdev.c | 1 +
>  drivers/net/enic/enic_main.c   | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> index cdf091559196..164f423a85c8 100644
> --- a/drivers/net/enic/enic_ethdev.c
> +++ b/drivers/net/enic/enic_ethdev.c
> @@ -834,6 +834,7 @@ static int enicpmd_dev_rss_hash_conf_get(struct 
> rte_eth_dev *dev,
>ENICPMD_FUNC_TRACE();
>if (rss_conf == NULL)
>return -EINVAL;
> +
>

unintended change.

also 'enicpmd_dev_configure()' looks like can be updated.


>if (rss_conf->rss_key != NULL &&
>rss_conf->rss_key_len < ENIC_RSS_HASH_KEY_SIZE) {
>dev_err(enic, "rss_hash_conf_get: wrong rss_key_len. given=%u"
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 19a99a82c501..2eafe7637b3a 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -1428,6 +1428,9 @@ int enic_set_rss_conf(struct enic *enic, struct 
> rte_eth_rss_conf *rss_conf)
>}
>}
>
> + if (rss_enable && rss_conf->algorithm != RTE_ETH_HASH_FUNCTION_DEFAULT)
> + return -EINVAL;
> +
>ret = enic_set_niccfg(enic, ENIC_RSS_DEFAULT_CPU, rss_hash_type,
>  ENIC_RSS_HASH_BITS, ENIC_RSS_BASE_CPU,
>  rss_enable);

Re: [PATCH 1/2] drivers: use macro for PCI address format

[John Daley (johndale)]

Acked-by: John Daley 


From: Thomas Monjalon 
Date: Thursday, October 19, 2023 at 8:41 AM
To: dev@dpdk.org 
Cc: David Marchand , chen...@nvidia.com 
, Jerin Jacob , Michal Krawczyk 
, Shai Brandes , Evgeny Schemeilin 
, Igor Chauskin , Ron Beider 
, John Daley (johndale) , Hyong Youb 
Kim (hyonkim) , Ziyang Xuan , 
Xiaoyun Wang , Guoyang Zhou 
, Matan Azrad , Viacheslav Ovsiienko 
, Ori Kam , Suanming Mou 
, Maciej Czekaj 
Subject: [PATCH 1/2] drivers: use macro for PCI address format
Some places were not using the macro PCI_PRI_FMT
as print format of a PCI address.

Note: RTE prefix is missing in the name of some PCI macros.

Signed-off-by: Thomas Monjalon 
---
 drivers/event/skeleton/skeleton_eventdev.c | 2 +-
 drivers/net/ena/ena_ethdev.c   | 2 +-
 drivers/net/enic/enic.h| 3 +--
 drivers/net/enic/enic_ethdev.c | 2 +-
 drivers/net/enic/enic_vf_representor.c | 2 +-
 drivers/net/hinic/hinic_pmd_ethdev.c   | 4 ++--
 drivers/net/mlx5/linux/mlx5_os.c   | 2 +-
 drivers/net/thunderx/nicvf_ethdev.c| 2 +-
 8 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/event/skeleton/skeleton_eventdev.c 
b/drivers/event/skeleton/skeleton_eventdev.c
index dc9b131641..dd2dab2e27 100644
--- a/drivers/event/skeleton/skeleton_eventdev.c
+++ b/drivers/event/skeleton/skeleton_eventdev.c
@@ -373,7 +373,7 @@ skeleton_eventdev_init(struct rte_eventdev *eventdev)
 skel->subsystem_device_id = pci_dev->id.subsystem_device_id;
 skel->subsystem_vendor_id = pci_dev->id.subsystem_vendor_id;

-   PMD_DRV_LOG(DEBUG, "pci device (%x:%x) %u:%u:%u:%u",
+   PMD_DRV_LOG(DEBUG, "PCI device (%x:%x) " PCI_PRI_FMT,
 pci_dev->id.vendor_id, pci_dev->id.device_id,
 pci_dev->addr.domain, pci_dev->addr.bus,
 pci_dev->addr.devid, pci_dev->addr.function);
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 7345e480f8..b39ec629e7 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -2125,7 +2125,7 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)

 pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);

-   PMD_INIT_LOG(INFO, "Initializing %x:%x:%x.%d\n",
+   PMD_INIT_LOG(INFO, "Initializing " PCI_PRI_FMT "\n",
  pci_dev->addr.domain,
  pci_dev->addr.bus,
  pci_dev->addr.devid,
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 163a1f037e..78778704f2 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -32,7 +32,6 @@

 #define ENICPMD_SETTING(enic, f) ((enic->config.flags & VENETF_##f) ? 1 : 0)

-#define ENICPMD_BDF_LENGTH  13   /* :00:00.0'\0' */
 #define ENIC_CALC_IP_CKSUM  1
 #define ENIC_CALC_TCP_UDP_CKSUM 2
 #define ENIC_MAX_MTU9000
@@ -101,7 +100,7 @@ struct enic {
 bool overlay_offload;
 struct rte_eth_dev *rte_dev;
 struct rte_eth_dev_data *dev_data;
-   char bdf_name[ENICPMD_BDF_LENGTH];
+   char bdf_name[PCI_PRI_STR_SIZE];
 int dev_fd;
 int iommu_group_fd;
 int iommu_groupid;
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index a487256fa1..b04b6c9aa1 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1274,7 +1274,7 @@ static int eth_enic_dev_init(struct rte_eth_dev *eth_dev,
 enic->pdev = pdev;
 addr = &pdev->addr;

-   snprintf(enic->bdf_name, ENICPMD_BDF_LENGTH, "%04x:%02x:%02x.%x",
+   snprintf(enic->bdf_name, PCI_PRI_STR_SIZE, PCI_PRI_FMT,
 addr->domain, addr->bus, addr->devid, addr->function);

 err = enic_check_devargs(eth_dev);
diff --git a/drivers/net/enic/enic_vf_representor.c 
b/drivers/net/enic/enic_vf_representor.c
index 46f85964e9..5d8d29135c 100644
--- a/drivers/net/enic/enic_vf_representor.c
+++ b/drivers/net/enic/enic_vf_representor.c
@@ -707,7 +707,7 @@ int enic_vf_representor_init(struct rte_eth_dev *eth_dev, 
void *init_params)
 LIST_INIT(&vf_enic->memzone_list);
 rte_spinlock_init(&vf_enic->memzone_list_lock);
 addr = &vf->bdf;
-   snprintf(vf_enic->bdf_name, ENICPMD_BDF_LENGTH, "%04x:%02x:%02x.%x",
+   snprintf(vf_enic->bdf_name, PCI_PRI_STR_SIZE, PCI_PRI_FMT,
  addr->domain, addr->bus, addr->devid, addr->function);
 return 0;
 }
diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c 
b/drivers/net/hinic/hinic_pmd_ethdev.c
index adc9f75c81..d4978e0649 100644
--- a/drivers/net/hinic/hinic_pmd_ethdev.c
+++ b/drivers/net/hinic/hinic_pmd_ethdev.c
@@ -3086,7 +3086,7 @@ static int hinic_func_init(struct rte_eth_dev *eth_dev)

 snprintf(nic_dev->proc_dev_

Re: [PATCH 6/7] enic: replace zero length array with flex array

[John Daley (johndale)]

Acked-by: John Daley 

From: Stephen Hemminger 
Date: Friday, January 13, 2023 at 1:52 PM
To: dev@dpdk.org 
Cc: Stephen Hemminger , John Daley (johndale) 
, Hyong Youb Kim (hyonkim) 
Subject: [PATCH 6/7] enic: replace zero length array with flex array
Zero length arrays are GNU extension. Replace with
standard flex array.

Signed-off-by: Stephen Hemminger 
---
 drivers/net/enic/base/vnic_devcmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/enic/base/vnic_devcmd.h 
b/drivers/net/enic/base/vnic_devcmd.h
index 253a791c3f65..f91cc3078d63 100644
--- a/drivers/net/enic/base/vnic_devcmd.h
+++ b/drivers/net/enic/base/vnic_devcmd.h
@@ -765,7 +765,7 @@ struct vnic_devcmd_notify {
 struct vnic_devcmd_provinfo {
 uint8_t oui[3];
 uint8_t type;
-   uint8_t data[0];
+   uint8_t data[];
 };

 /*
--
2.39.0

RE: [PATCH] net/enic: adjust memory check and use in proper order

[John Daley (johndale)]

Reviewed-by: John Daley 

Thanks,
John

-Original Message-
From: Weiguo Li  
Sent: Tuesday, January 25, 2022 4:01 AM
To: John Daley (johndale) 
Cc: dev@dpdk.org
Subject: [PATCH] net/enic: adjust memory check and use in proper order

Fixes: bb66d562aefc ("net/enic: share flow actions with same signature")

Signed-off-by: Weiguo Li 
---
 drivers/net/enic/enic_fm_flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/enic/enic_fm_flow.c b/drivers/net/enic/enic_fm_flow.c 
index bf04d714d0..d8718d17ef 100644
--- a/drivers/net/enic/enic_fm_flow.c
+++ b/drivers/net/enic/enic_fm_flow.c
@@ -2521,11 +2521,11 @@ enic_action_handle_get(struct enic_flowman *fm, struct 
fm_action *action_in,
memcpy(fma, action_in, sizeof(*fma));
 
ah = calloc(1, sizeof(*ah));
-   memcpy(&ah->key, action_in, sizeof(struct fm_action));
if (ah == NULL)
return rte_flow_error_set(error, ENOMEM,
   RTE_FLOW_ERROR_TYPE_HANDLE,
   NULL, "enic: calloc(fm-action)");
+   memcpy(&ah->key, action_in, sizeof(struct fm_action));
args[0] = FM_ACTION_ALLOC;
args[1] = fm->cmd.pa;
ret = flowman_cmd(fm, args, 2);
--
2.25.1

Re: [PATCH] net/enic: avoid extra unlock when setting MTU in enic

[John Daley (johndale)]

Reviewed-by: John Daley 

Thanks,
John

From: Weiguo Li 
Date: Wednesday, November 1, 2023 at 12:28 AM
To: John Daley (johndale) 
Cc: dev@dpdk.org , sta...@dpdk.org , Weiguo Li 

Subject: [PATCH] net/enic: avoid extra unlock when setting MTU in enic
The 'set_mtu_done' goto statement is being executed in a context
where the 'mtu_lock' has not been previously locked.

To avoid the extra unlocking operation, replace the goto statement
with a return statement.

Fixes: c3e09182bcd6 ("net/enic: support scatter Rx in MTU update")
Cc: sta...@dpdk.org

Signed-off-by: Weiguo Li 
---
 .mailmap | 2 +-
 drivers/net/enic/enic_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index 3f5bab26a8..b4f0ae26b8 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1500,7 +1500,7 @@ Waterman Cao 
 Weichun Chen 
 Wei Dai 
 Weifeng Li 
-Weiguo Li 
+Weiguo Li  
 Wei Huang 
 Wei Hu 
 Wei Hu (Xavier) 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 19a99a82c5..a6aaa760ca 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1639,7 +1639,7 @@ int enic_set_mtu(struct enic *enic, uint16_t new_mtu)
  * packet length.
  */
 if (!eth_dev->data->dev_started)
-   goto set_mtu_done;
+   return rc;

 /*
  * The device has started, re-do RQs on the fly. In the process, we
--
2.34.1

Re: [Bug 1185] enic: no longer accepting 2048 descriptor size in 20.11.6

[John Daley (johndale)]

Thank you, I will take a look and get back to you.
-john

From: Kevin Traynor 
Date: Wednesday, March 15, 2023 at 10:47 AM
To: John Daley (johndale) , Hyong Youb Kim (hyonkim) 

Cc: Luca Boccassi , David Marchand 
, dev@dpdk.org 
Subject: Fwd: [Bug 1185] enic: no longer accepting 2048 descriptor size in 
20.11.6
Hi John/Hyong Youb,

Not sure if you are monitoring dpdk bugzilla, but wanted to draw your
attention to https://bugs.dpdk.org/show_bug.cgi?id=1185

Maybe you have some clues about it and if we need to backport the
mentioned commit?

thanks,
Kevin.

 Forwarded Message 
Subject: [Bug 1185] enic: no longer accepting 2048 descriptor size in
20.11.6
Date: Wed, 15 Mar 2023 17:33:52 +
From: bugzi...@dpdk.org
To: dev@dpdk.org

https://bugs.dpdk.org/show_bug.cgi?id=1185

 Bug ID: 1185
Summary: enic: no longer accepting 2048 descriptor size in
 20.11.6
Product: DPDK
Version: 20.11
   Hardware: All
 OS: All
 Status: UNCONFIRMED
   Severity: normal
   Priority: Normal
  Component: core
   Assignee: dev@dpdk.org
   Reporter: ktray...@redhat.com
   Target Milestone: ---

Hi enic maintainers,

With openvswitch 2.15 using dpdk 20.11.6, enic driver is reporting that it
cannot accept setup with default OVS number of tx descriptors (2048).

2022-08-19T16:29:38.555Z|00204|dpdk|ERR|Invalid value for nb_tx_desc(=2048),
should be: <= 256, >= 64, and a product of 32

OVS has used 2048 as default for many years and the code in 20.11.6 does not
look like it changed much from previous releases either.

The commit below [0] on dpdk main branch (but not on 20.11 branch),
changes how
max descriptor values are calculated.

So any idea why the 2048 can no longer be used with 20.11.6 ? some commit I
missed? different firmware?

or is it an incorrect calculation and the commit [0] is needed on stable
branches to correct this?

Thanks.

Reference: https://bugzilla.redhat.com/show_bug.cgi?id=2119876

[0]
commit 22572e84fbda2c195707ffbb0dd6af4433d7a219
Author: John Daley 
Date:   Fri Jan 28 09:58:13 2022 -0800

 net/enic: support max descriptors allowed by adapter

 Newer VIC adapters have the max number of supported RX and TX
 descriptors in their configuration. Use these values as the
 maximums.

 Signed-off-by: John Daley 
 Reviewed-by: Hyong Youb Kim 

--
You are receiving this mail because:
You are the assignee for the bug.

Re: [PATCH v2 13/44] net/enic: fix segment fault when parse devargs

[John Daley (johndale)]

Reviewed-by: John Daley johnd...@cisco.com<mailto:johnd...@cisco.com>

From: Chengwen Feng 
Date: Monday, March 20, 2023 at 2:28 AM
To: tho...@monjalon.net , ferruh.yi...@amd.com 
, John Daley (johndale) , Hyong Youb 
Kim (hyonkim) 
Cc: dev@dpdk.org 
Subject: [PATCH v2 13/44] net/enic: fix segment fault when parse devargs
The rte_kvargs_process() was used to parse KV pairs, it also supports
to parse 'only keys' (e.g. socket_id) type. And the callback function
parameter 'value' is NULL when parsed 'only keys'.

This patch fixes segment fault when parse input args with 'only keys'.

Fixes: 93fb21fdbe23 ("net/enic: enable overlay offload for VXLAN and GENEVE")
Fixes: e39c2756e21a ("net/enic: add devarg to specify ingress VLAN rewrite 
mode")
Cc: sta...@dpdk.org

Signed-off-by: Chengwen Feng 
---
 drivers/net/enic/enic_ethdev.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index cdf0915591..b67016e0a3 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1148,6 +1148,9 @@ static int enic_parse_zero_one(const char *key,
 struct enic *enic;
 bool b;

+   if (value == NULL)
+   return -EINVAL;
+
 enic = (struct enic *)opaque;
 if (strcmp(value, "0") == 0) {
 b = false;
@@ -1173,6 +1176,9 @@ static int enic_parse_ig_vlan_rewrite(__rte_unused const 
char *key,
 {
 struct enic *enic;

+   if (value == NULL)
+   return -EINVAL;
+
 enic = (struct enic *)opaque;
 if (strcmp(value, "trunk") == 0) {
 /* Trunk mode: always tag */
--
2.17.1

Re: [dpdk-dev] [PATCH] net/enic: fix MAC address add and remove

[John Daley (johndale)]

> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Friday, January 27, 2017 3:07 AM
> To: John Daley (johndale) 
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/enic: fix MAC address add and remove
> 
> On 1/26/2017 8:12 PM, John Daley wrote:
> > The mac_addr_add callback function was simply replacing the primary
> > MAC address instead of adding new ones and the mac_addr_remove
> > callback would only remove the primary MAC form the adapter. Fix the
> > functions to add or remove new address. Allow up to 64 MAC addresses
> per port.
> >
> > Fixes: fefed3d1e62c ("enic: new driver")
> >
> > Signed-off-by: John Daley 
> > Reviewed-by: Nelson Escobar 
> 
> Applied to dpdk-next-net/master, thanks.
> 
> (Since it is controversial that if this patch is a fix or implementing a 
> missing
> feature (allow up to 64 MAC), I am getting patch as it is, but not CC'ed to
> stable tree, please shout if you disagree.)

That's fine to not put it on stable. Thank you for applying.
-john

Re: [dpdk-dev] [PATCH v2] net/enic: fix hardcoding of some flow director masks

[John Daley (johndale)]

> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Thursday, February 09, 2017 2:16 AM
> To: John Daley (johndale) 
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [PATCH v2] net/enic: fix hardcoding of some flow director masks
> 
> On 2/9/2017 12:40 AM, John Daley wrote:
> > Hard coded mask values were being used for several of the IPv4 and
> > IPv6 fields. Use the values in the rte_eth_fdir_masks structure
> > provided by the caller.
> >
> > Fixes: dfbd6a9cb504 ("net/enic: extend flow director support for 1300
> > series")
> >
> > Cc: sta...@dpdk.org
> > Signed-off-by: John Daley 
> > ---
> >
> >  v2: fix compile error
> 
> I wasn't getting an error for this.
> 
> V2 adds:
> 
> - ipv6_val.vtc_flow = input->flow.ipv6_flow.tc << 16;
> + ipv6_val.vtc_flow = input->flow.ipv6_flow.tc << 12;
> 
> What is the compile error this fixes?

This patch had the compile error:
http://www.dpdk.org/dev/patchwork/patch/20147/
+   ipv6_mask.vtc_flow = masks->ipv6_mask.tc << 16);

Re: [dpdk-dev] [PATCH v2] enic flow director fixes

[John Daley (johndale)]

> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Thursday, February 09, 2017 4:15 AM
> To: John Daley (johndale) 
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] enic flow director fixes
> 
> On 2/9/2017 12:40 AM, John Daley wrote:
> > This should replace
> > http://www.dpdk.org/dev/patchwork/patch/20147/ which was an
> > intermediate patch in a small patchset and had a compile error. Also,
> > please reject the 2nd patch in the patchset
> > http://www.dpdk.org/dev/patchwork/patch/20148/ which is incomplete. I
> will change the state of these patches in patchworks.
> > thank you, john
> 
> Hi John,
> 
> I dropped the previous two and will get this one. And this looks like the
> squashed version of the previous two with some updates in ipv6 shift
> number.
> 
> Although next-net can re-write the git history, this is mainly to provide 
> ability
> to rebasing on master tree. Not for amending patches on fly, that is
> confusing.
> 
> Please next time do not rely on git history re-writing while sending patches.

Oops I was stupidly looking at master and assumed the patches were rejected 
because of the compile error. I obviously should have been looking at next-net.

Thanks,
John
> 
> Thanks,
> ferruh
> 
> >
> > John Daley (1):
> >   net/enic: fix hardcoding of some flow director masks
> >
> >  drivers/net/enic/enic_clsf.c | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >

Re: [dpdk-dev] [PATCH] enic: check for nb_free > 0

[John Daley (johndale)]

> -Original Message-
> From: Aaron Conole [mailto:acon...@redhat.com]
> Sent: Wednesday, August 02, 2017 11:02 AM
> To: dev@dpdk.org
> Cc: sta...@dpdk.org; John Daley (johndale) ; Bruce
> Richardson 
> Subject: [PATCH] enic: check for nb_free > 0
> 
> Occasionally, the amount of packets to free from the work queue ends
> perfectly on a boundary to have nb_free = 0 and pool = 0.  This causes a
> segfault as follows:
> 
>   (gdb) bt
>   #0  rte_mempool_default_cache (mp=, mp= out>,
>   lcore_id=)
>   at /usr/src/debug/openvswitch-2.6.1/dpdk-16.11/x86_64-native-
> linuxapp-gcc/include/rte_mempool.h:1017
>   #1  rte_mempool_put_bulk (n=0, obj_table=0x7f10deff2530, mp=0x0)
>   at /usr/src/debug/openvswitch-2.6.1/dpdk-16.11/x86_64-native-
> linuxapp-gcc/include/rte_mempool.h:1174
>   #2  enic_free_wq_bufs (wq=wq@entry=0x7efabffcd5b0,
>   completed_index=completed_index@entry=33)
>   at /usr/src/debug/openvswitch-2.6.1/dpdk-
> 16.11/drivers/net/enic/enic_rxtx.c:429
>   #3  0x7f11e9c86e17 in enic_cleanup_wq (enic=,
>   wq=wq@entry=0x7efabffcd5b0)
>   at /usr/src/debug/openvswitch-2.6.1/dpdk-
> 16.11/drivers/net/enic/enic_rxtx.c:442
>   #4  0x7f11e9c86e5f in enic_xmit_pkts (tx_queue=0x7efabffcd5b0,
>   tx_pkts=0x7f10deffb1a8, nb_pkts=)
>   at /usr/src/debug/openvswitch-2.6.1/dpdk-
> 16.11/drivers/net/enic/enic_rxtx.c:470
>   #5  0x7f11e9e147ad in rte_eth_tx_burst (nb_pkts=,
>   tx_pkts=0x7f10deffb1a8, queue_id=0, port_id=)
> 
> This commit makes the enic wq driver match other drivers who call the bulk
> free, by checking that there are actual packets to free.
> 
> Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
> CC: sta...@dpdk.org
> Reported-by: Vincent S. Cojot 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1468631
> Signed-off-by: Aaron Conole 
> ---
>  drivers/net/enic/enic_rxtx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c index
> 5867acf..a39172f 100644
> --- a/drivers/net/enic/enic_rxtx.c
> +++ b/drivers/net/enic/enic_rxtx.c
> @@ -503,7 +503,8 @@ static inline void enic_free_wq_bufs(struct vnic_wq
> *wq, u16 completed_index)
>   tail_idx = enic_ring_incr(desc_count, tail_idx);
>   }
> 
> - rte_mempool_put_bulk(pool, (void **)free, nb_free);
> + if (nb_free > 0)
> + rte_mempool_put_bulk(pool, (void **)free, nb_free);
> 
>   wq->tail_idx = tail_idx;
>   wq->ring.desc_avail += nb_to_free;
> --
> 2.9.4

Reviewed-by: John Daley 

Thank you!

johnd

Re: [dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics

[John Daley (johndale)]

Hi Adrien,

> -Original Message-
> From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com]
> Sent: Friday, March 24, 2017 2:47 AM
> To: John Daley (johndale) 
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 0/1] proposed minor change in rte_flow_validate
> semantics
> 
> Hi John,
> 
> On Thu, Mar 23, 2017 at 07:36:58PM -0700, John Daley wrote:
> > Hi,
> >
> > In implementing rte_flow_validate() for the Cisco enic, I got to
> > wondering if the semantics might be slightly off given how I see apps using
> it.
> >
> > Please forgive me if this has already been discussed, but during
> > runtime, I can't see any reason why rte_flow_validate() would be
> > called when
> > rte_flow_create() returns the same errors as validate, but also
> > creates the filter if all is good (calling validate first is just extra 
> > overhead).
> > What are the use cases for calling validate when the app is up and running?
> 
> In short (I'll elaborate below), to get a rough idea of the capabilities of a 
> given
> port at initialization time. One should use rte_flow_validate() with
> nonspecific rules to determine if the PMD understands them at all *given the
> circumstances* (the current configuration state of the device).
> 
> Nothing prevents one from using it just before calling rte_flow_create(),
> however doing so is indeed redundant.
> 
> > I see rte_flow_validate() being run at startup for answering these 2
> questions:
> > 1.) Given enough resources, is the device capable of handling flows a,
> > b, c, ..,  which the app may want to create during runtime?
> 
> Yes, breaking this down means all the following:
> 
> 1. Recognize all pattern items and actions.
> 2. Make sure items are properly stacked and actions can be combined.
> 3. Check for pattern item specification structures (e.g. limits, masks,
>ranges, and so on).
> 4. Check for actions configuration structures (e.g. target queue exists).
> 
> Depending on the flow rule, 3. and 4. may have to check the current device
> configuration in order to validate the rule.

Agreed. You didn't include checking for duplicate patterns with conflicting 
actions. Is that a requirement also? I hope some of that and some of 1, 2 and 3 
can be pushed up into rte_ether/flow someday. I.e. only present flows to the 
PMDs which are already valid, so the PMD just needs to see if it is acceptable 
to the device.

> 
> > 2.) How many concurrent flows of type a, b, c, .. can the device
> > handle? To answer this app needs to do a bunch of rte_flow_create()'s
> (followed by a bunch of rte_flow_destroy()s).
> 
> Hehe, this is the brute force approach, actually the rte_flow API in its 
> current
> form is not supposed to answer this question, and I don't think we should
> document it that way (note you could use rte_flow_flush() to destroy all
> flows at once faster).
> 
> Some devices (like mlx5) do not really have an upper limit on the number of
> possible flows one can configure, they're basically limited by the available
> memory on the host system.
> 
> > The answer to these questions will allow the app to decide beforehand
> > which filters to offload (if any) and set up function pointers or
> > whatever to run as efficiently as possible on the target device.
> >
> > rte_flow_validate() has comments that imply it should be run on the
> > fly,like "the flow rule is validated against its current configuration
> > state and the returned value should be considered valid by the caller for
> that state only".
> > The only real way to implement this is for enic (and I think other
> > nics) is to actually do a flow create/destroy within
> > rte_flow_validate(). But, see paragraph 2 above- why bother? I looked
> > at the i40e and mlx5 implementations and I didn't see any calls to the
> > nics to check a flow against current state so they might not be
> implemented as the comment imply they should be either.
> 
> Then you're probably right there is an issue with the documentation, it's not
> the intent (more below).
> 
> > If there are no use cases for calling validate at runtime,  I think we
> > ought to change the semantics of rte_flow_validate() a little to
> > define success to mean that the device will accept the flow IF there
> > are enough resources, rather than it will accept the flow given the
> > current state. We can safely remove the -EEXIST return code since no
> > other drivers set them yet. This makes the function easier to
> > implement correctly without trying to do something that's not useful
> anyway.
> >
> > The following patc

[dpdk-dev] [opnfv-tech-discuss][apex][ovsnfv]Problem showed up with OVS/DPDK with Cisco VIC adapter

[John Daley (johndale)]

Hi,
Please see inline.
Thanks,
john

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas F Herbert
> Sent: Tuesday, October 18, 2016 1:35 PM
> To: dev at dpdk.org
> Cc: Keith Burns ; Edward Warnicke
> ; opnfv-tech-discuss at lists.opnfv.org
> Subject: [dpdk-dev] [opnfv-tech-discuss][apex][ovsnfv]Problem showed up
> with OVS/DPDK with Cisco VIC adapter
> 
> All:
> 
> This is not necessarily related to VPP but rather to OVS/DPDK.
> In OPNFV we found the following problem when using UCS NIC.
> The UCS fabric seems to set a VLAN tag on untagged packets.
> Any thoughts from DPDK and VPP folks would be appreciated.
> 
In a UCS fabric, all frames between the VIC and the Fabric Interconnect will be 
tagged. This is required to carry both VLAN information and, being a converged 
adapter supporting both Ethernet and FCoE, traffic class. For non-UCS fabric 
deployments, there is currently no way to turn off egress priority tagging on 
the VIC adapter. If a packet being sent from DPDK to the enic PMD is priority 
tagged (VLAN=0) or has no VLAN tag, the default VLAN tag (as set up in 
CIMC/UCSM manager) will be inserted.  This should only be an issue with 
C-series UCS servers connected point to point or through a switch that can't 
cope with priority tags. Is that the case here?

> ...
> --
> *Thomas F Herbert*
> SDN Group
> Office of Technology
> *Red Hat*

Re: [dpdk-dev] [PATCH] ethdev: add isolated mode to flow API

[John Daley (johndale)]

Hi Adrien,

I understand why applications may want to operate in this sort of isolated 
mode. Just to better understand the value of adding this feature can you 
contrast it to simply having the application add a very low priority "catch 
all" flow rule with DROP action?

Thanks,
Johnd

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Friday, May 19, 2017 1:14 AM
> To: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add isolated mode to flow API
> 
> Re-sending this due to the lack of feedback. Anyone?
> 
> On Thu, Mar 30, 2017 at 11:08:16AM +0200, Adrien Mazarguil wrote:
> > Isolated mode can be requested by applications on individual ports to
> > avoid ingress traffic outside of the flow rules they define.
> >
> > Besides making ingress more deterministic, it allows PMDs to safely
> > reuse resources otherwise assigned to handle the remaining traffic,
> > such as global RSS configuration settings, VLAN filters, MAC address
> > entries, legacy filter API rules and so on in order to expand the set
> > of possible flow rule types.
> >
> > To minimize code complexity, PMDs implementing this mode may provide
> > partial (or even no) support for flow rules when not enabled (e.g. no
> > priorities, no RSS action). Applications written to use the flow API
> > are therefore encouraged to enable it.
> >
> > Once effective, leaving isolated mode may not be possible depending on
> > PMD implementation.
> >
> > Signed-off-by: Adrien Mazarguil 
> > Cc: Ferruh Yigit 
> > Cc: John McNamara 
> > Cc: "O'Driscoll, Tim" 
> > Cc: "Lu, Wenzhuo" 
> > Cc: John Daley 
> > Cc: Konstantin Ananyev 
> > Cc: Helin Zhang 
> > Cc: Nelio Laranjeiro 
> > Cc: Andrew Rybchenko 
> > Cc: Pascal Mazon 
> > ---
> >  app/test-pmd/cmdline.c  |  4 ++
> >  app/test-pmd/cmdline_flow.c | 49 -
> >  app/test-pmd/config.c   | 16 +++
> >  app/test-pmd/testpmd.h  |  1 +
> >  doc/guides/prog_guide/rte_flow.rst  | 56
> 
> >  doc/guides/testpmd_app_ug/testpmd_funcs.rst | 47
> +++-
> >  drivers/net/ixgbe/ixgbe_flow.c  |  9 ++--
> >  lib/librte_ether/rte_ether_version.map  |  7 +++
> >  lib/librte_ether/rte_flow.c | 18 
> >  lib/librte_ether/rte_flow.h | 33 ++
> >  lib/librte_ether/rte_flow_driver.h  |  5 +++
> >  11 files changed, 238 insertions(+), 7 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > e0d54d4..dbab647 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -892,6 +892,10 @@ static void cmd_help_long_parsed(void
> *parsed_result,
> > "flow list {port_id} [group {group_id}] [...]\n"
> > "List existing flow rules sorted by priority,"
> > " filtered by group identifiers.\n\n"
> > +
> > +   "flow isolate {port_id} {boolean}\n"
> > +   "Restrict ingress traffic to the defined"
> > +   " flow rules\n\n"
> > );
> > }
> >  }
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index 4e99f0f..b783b81 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -80,6 +80,7 @@ enum index {
> > FLUSH,
> > QUERY,
> > LIST,
> > +   ISOLATE,
> >
> > /* Destroy arguments. */
> > DESTROY_RULE,
> > @@ -361,6 +362,9 @@ struct buffer {
> > uint32_t *group;
> > uint32_t group_n;
> > } list; /**< List arguments. */
> > +   struct {
> > +   int set;
> > +   } isolate; /**< Isolated mode arguments. */
> > } args; /**< Command arguments. */
> >  };
> >
> > @@ -631,6 +635,9 @@ static int parse_action(struct context *, const
> > struct token *,  static int parse_list(struct context *, const struct token 
> > *,
> >   const char *, unsigned int,
> >   void *, unsigned int);
> > +static int parse_isolate(struct context *, const struct token *,
> > +const char *, unsigned int,
> > +void *, unsigned int);
> >  static int parse_int(struct context *, const struct token *,
> >  const char *, unsigned int,
> >  void *, unsigned int);
> > @@ -777,7 +784,8 @@ static const struct token token_list[] = {
> >   DESTROY,
> >   FLUSH,
> >   LIST,
> > - QUERY)),
> > + QUERY,
> > + ISOLATE)),
> > .call = parse_init,
> > },
> > /* Sub-level commands. */
> > @@ -827,6 +835,15 @@ static const struct token token_list[] = {
> > .args = ARGS(ARGS_ENTRY(struct buffer, port)),
> > 

Re: [dpdk-dev] [PATCH] app/testpmd: fix flow query failure

[John Daley (johndale)]

Ok, makes sense now, both flow query and flow list work with this patch, yeah.

Tested-by: John Daley 

Remember that https://patches.dpdk.org/patch/46221/ still needs to be reverted 
out of dpdk-next-net/master otherwise we are still broken. The patch was 
squished into e5b652ea34.

-johnd

> -Original Message-
> From: dev  On Behalf Of Mordechay Haimovsky
> Sent: Wednesday, October 10, 2018 10:05 AM
> To: Adrien Mazarguil ; Shahaf Shuler
> ; Ori Kam 
> Cc: dev@dpdk.org; Mordechay Haimovsky 
> Subject: [dpdk-dev] [PATCH] app/testpmd: fix flow query failure
> 
> This patch fixes a bug found in port_flow_query routine which caused flow
> query command to fail with the following error "Caught error type 1 (cause
> unspecified): unknown object type to retrieve the name
> of: Invalid argument".
> 
> Fixes: f7ba5e7a0f8c ("app/testpmd: rely on flow API conversion function")
> 
> Signed-off-by: Moti Haimovsky 
> ---
>  app/test-pmd/config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 009c92c..b11317b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1239,7 +1239,8 @@ void print_valid_ports(void)
>   return -ENOENT;
>   }
>   ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> - &name, sizeof(name), action, &error);
> + &name, sizeof(name),
> + (void *)(uintptr_t)action->type, &error);
>   if (ret < 0)
>   return port_flow_complain(&error);
>   switch (action->type) {
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2 00/15] enic PMD fixes and performance improvements

[John Daley (johndale)]

Hi Kevin,
Inline.
-john

> -Original Message-
> From: Kevin Traynor 
> Sent: Wednesday, July 25, 2018 11:37 AM
> To: John Daley (johndale) ; ferruh.yi...@intel.com
> Cc: dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 00/15] enic PMD fixes and performance
> improvements
> 
> On 06/29/2018 10:29 AM, John Daley wrote:
> > Updated a few commits in the patchset per suggestions by Ferrus Yigit.
> > thanks,
> > John
> >
> > Hyong Youb Kim (12):
> >   net/enic: fix receive packet types
> >   net/enic: update the UDP RSS detection mechanism
> >   net/enic: do not overwrite admin Tx queue limit
> >   net/enic: initialize RQ fetch index before enabling RQ
> >   net/enic: report ring limits and preferred default values
> >   net/enic: add devarg to specify ingress VLAN rewrite mode
> >   net/enic: add handlers to add/delete vxlan port number
> >   net/enic: use mbuf pointer array for inflight Tx packets
> >   net/enic: support mbuf fast free offload
> >   net/enic: reduce Tx completion updates
> >   net/enic: add the simple version of Tx handler
> >   net/enic: check maximum packet size in Tx prepare handler
> >
> > John Daley (3):
> >   net/enic: add simple Rx handler
> >   net/enic: cap Rx packet processing to end of desc ring
> >   doc: update release notes with new enic features
> >
> 
> Hi John, seems like many of these would be candidates for stable branches?

Yes, a few of them. We will backport, test and submit to the stable branches as 
soon as we are done with 18.08 testing.
Thanks,
John

> 
> >  doc/guides/nics/enic.rst   |  15 +-
> >  doc/guides/nics/features/enic.ini  |   1 +
> >  doc/guides/rel_notes/release_18_08.rst |   8 +
> >  drivers/net/enic/base/cq_desc.h|   1 +
> >  drivers/net/enic/base/vnic_dev.c   |  16 ++
> >  drivers/net/enic/base/vnic_dev.h   |   4 +
> >  drivers/net/enic/base/vnic_devcmd.h|  23 ++-
> >  drivers/net/enic/base/vnic_enet.h  |   5 +-
> >  drivers/net/enic/base/vnic_nic.h   |   4 +-
> >  drivers/net/enic/base/vnic_rq.h|   2 +
> >  drivers/net/enic/base/vnic_wq.c|   9 +-
> >  drivers/net/enic/base/vnic_wq.h|  12 +-
> >  drivers/net/enic/enic.h|  12 ++
> >  drivers/net/enic/enic_compat.h |   5 +
> >  drivers/net/enic/enic_ethdev.c | 168 +++--
> >  drivers/net/enic/enic_main.c   | 125 ++---
> >  drivers/net/enic/enic_res.c|  13 +-
> >  drivers/net/enic/enic_res.h|  16 ++
> >  drivers/net/enic/enic_rxtx.c   | 333 
> > +---
> -
> >  19 files changed, 668 insertions(+), 104 deletions(-)
> >

Re: [dpdk-dev] Survey for final decision about per-port offload API

[John Daley (johndale)]

Hi,
Inline.
Thanks,
John

> -Original Message-
> From: Thomas Monjalon 
> Sent: Friday, March 30, 2018 6:48 AM
> To: dev@dpdk.org
> Cc: Ajit Khaparde ; Jerin Jacob
> ; Shijith Thotton
> ; Santosh Shukla
> ; Rahul Lakkireddy
> ; John Daley (johndale) ;
> Wenzhuo Lu ; Konstantin Ananyev
> ; Beilei Xing ; Qi Zhang
> ; Jingjing Wu ; Adrien Mazarguil
> ; Nelio Laranjeiro
> ; Yongseok Koh ; Shahaf
> Shuler ; Tomasz Duszynski ;
> Jianbo Liu ; Alejandro Lucero
> ; Hemant Agrawal
> ; Shreyansh Jain ;
> Harish Patil ; Rasesh Mody
> ; Andrew Rybchenko
> ; Shrikrishna Khare ;
> Maxime Coquelin ; Allain Legacy
> ; Bruce Richardson
> ; Gaetan Rivet ;
> Olivier Matz 
> Subject: Survey for final decision about per-port offload API
> 
> There are some discussions about a specific part of the offload API:
>   "To enable per-port offload, the offload should be set on both
>   device configuration and queue setup."
> 
> It means the application must repeat the port offload flags in
> rte_eth_conf.[rt]xmode.offloads and rte_eth_[rt]xconf.offloads, when calling
> respectively rte_eth_dev_configure() and rte_eth_[rt]x_queue_setup for each
> queue.
> 
> The PMD must check if there is mismatch, i.e. a port offload not repeated in
> queue setup.
> There is a proposal to do this check at ethdev level:
>   http://dpdk.org/ml/archives/dev/2018-March/094023.html
> 
> It was also proposed to relax the API and allow "forgetting" port offloads in
> queue offloads:
>   http://dpdk.org/ml/archives/dev/2018-March/092978.html
> 
> It would mean the offloads applied to a queue result of OR operation:
>   rte_eth_conf.[rt]xmode.offloads | rte_eth_[rt]xconf.offloads
> 
> 1/ Do you agree with above API change?

YES

> 
> 
> If we agree with this change, we need to update the documentation and remove
> the checks in PMDs.
> Note: no matter what is decided here, 18.05-rc1 should have all PMDs switched
> to the API which was defined in 17.11.
> Given that API is new and not yet adopted by the applications, the sonner it 
> is
> fixed, the better.
> 
> 2/ Should we do this change in 18.05-rc2?
> 
> 
YES

> At the same time, we want to make clear that an offload enabled at port level,
> cannot be disabled at queue level.
> 
> 3/ Do you agree with above statement (to be added in the doc)?

YES

> 
> 
> There is the same kind of confusion in the offload capabilities:
>   rte_eth_dev_info.[rt]x_offload_capa
>   rte_eth_dev_info.[rt]x_queue_offload_capa
> The queue capabilities must be a subset of port capabilities, i.e. every queue
> capabilities must be reported as port capabilities.
> But the port capabilities should be reported at queue level only if it can be
> applied to a specific queue.
> 
> 4/ Do you agree with above statement (to be added in the doc)?

YES
> 
> 
> Please give your opinion on questions 1, 2, 3 and 4.
> Answering by yes/no may be sufficient in most cases :) Thank you
> 

Re: [dpdk-dev] [RFC] tunnel endpoint hw acceleration enablement

[John Daley (johndale)]

Hi,
One comment on DECAP action and a "feature request".  I'll also reply to the 
top of thread discussion separately. Thanks for the RFC Declan!

Feature request associated with ENCAP action:

VPP (and probably other apps) would like the ability to simply specify an 
independent tunnel ID as part of egress match criteria in an rte_flow rule. 
Then egress packets could specify a tunnel ID  and valid flag in the mbuf. If 
it matched the rte_flow tunnel ID item, a simple lookup in the nic could be 
done and the associated actions (particularly ENCAP) executed. The application 
already know the tunnel that the packet is associated with so no need to have 
the nic do matching on a header pattern. Plus it's possible that packet headers 
alone are not enough to determine the correct encap action (the bridge where 
the packet came from might be required). 

This would require a new mbuf field to specify the tunnel ID (maybe in 
tx_offload) and a valid flag.  It would also require a new rte flow item type 
for matching the tunnel ID (like RTE_FLOW_ITEM_TYPE_META_TUNNEL_ID).

Is something like this being considered by others? If not, should it be part of 
this RFC or a new one? I think this would be the 1st meta-data match criteria 
in rte_flow, but I could see others following. 

-johnd

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Doherty, Declan
> Sent: Thursday, December 21, 2017 2:21 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC] tunnel endpoint hw acceleration enablement
> 
> This RFC contains a proposal to add a new tunnel endpoint API to DPDK that
> when used in conjunction with rte_flow enables the configuration of inline
> data path encapsulation and decapsulation of tunnel endpoint network
> overlays on accelerated IO devices.
> 
> The proposed new API would provide for the creation, destruction, and
> monitoring of a tunnel endpoint in supporting hw, as well as capabilities APIs
> to allow the acceleration features to be discovered by applications.
> 
> /** Tunnel Endpoint context, opaque structure */ struct rte_tep;
> 
> enum rte_tep_type {
>RTE_TEP_TYPE_VXLAN = 1, /**< VXLAN Protocol */
>RTE_TEP_TYPE_NVGRE, /**< NVGRE Protocol */
>...
> };
> 
> /** Tunnel Endpoint Attributes */
> struct rte_tep_attr {
>enum rte_type_type type;
> 
>/* other endpoint attributes here */ }
> 
> /**
> * Create a tunnel end-point context as specified by the flow attribute and
> pattern
> *
> * @param   port_id Port identifier of Ethernet device.
> * @param   attrFlow rule attributes.
> * @param   pattern Pattern specification by list of rte_flow_items.
> * @return
> *  - On success returns pointer to TEP context
> *  - On failure returns NULL
> */
> struct rte_tep *rte_tep_create(uint16_t port_id,
>   struct rte_tep_attr *attr, struct rte_flow_item 
> pattern[])
> 
> /**
> * Destroy an existing tunnel end-point context. All the end-points context
> * will be destroyed, so all active flows using tep should be freed before
> * destroying context.
> * @param   port_idPort identifier of Ethernet device.
> * @param   tepTunnel endpoint context
> * @return
> *  - On success returns 0
> *  - On failure returns 1
> */
> int rte_tep_destroy(uint16_t port_id, struct rte_tep *tep)
> 
> /**
> * Get tunnel endpoint statistics
> *
> * @param   port_idPort identifier of Ethernet device.
> * @param   tepTunnel endpoint context
> * @param   stats  Tunnel endpoint statistics
> *
> * @return
> *  - On success returns 0
> *  - On failure returns 1
> */
> Int
> rte_tep_stats_get(uint16_t port_id, struct rte_tep *tep,
>   struct rte_tep_stats *stats)
> 
> /**
> * Get ports tunnel endpoint capabilities
> *
> * @param   port_idPort identifier of Ethernet device.
> * @param   capabilitiesTunnel endpoint capabilities
> *
> * @return
> *  - On success returns 0
> *  - On failure returns 1
> */
> int
> rte_tep_capabilities_get(uint16_t port_id,
>   struct rte_tep_capabilities *capabilities)
> 
> 
> To direct traffic flows to hw terminated tunnel endpoint the rte_flow API is
> enhanced to add a new flow item type. This contains a pointer to the TEP
> context as well as the overlay flow id to which the traffic flow is 
> associated.
> 
> struct rte_flow_item_tep {
>struct rte_tep *tep;
>uint32_t flow_id;
> }
> 
> Also 2 new generic actions types are added encapsulation and decapsulation.
> 
> RTE_FLOW_ACTION_TYPE_ENCAP
> RTE_FLOW_ACTION_TYPE_DECAP
> 
> struct rte_flow_action_encap {
>struct rte_flow_item *item; }
> 
> struct rte_flow_action_decap {
>struct rte_flow_item *item; }
> 
> The following section outlines the intended usage of the new APIs and then
> how they are combined with the existing rte_flow APIs.
> 
> Tunnel endp

Re: [dpdk-dev] [RFC] tunnel endpoint hw acceleration enablement

[John Daley (johndale)]

Hi Declan and Shahaf,

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Doherty, Declan
> Sent: Tuesday, January 09, 2018 9:31 AM
> To: Shahaf Shuler ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC] tunnel endpoint hw acceleration enablement
> 
> On 24/12/2017 5:30 PM, Shahaf Shuler wrote:
> > Hi Declan,
> >
> 
> Hey Shahaf, apologies for the delay in responding, I have been out of office
> for the last 2 weeks.
> 
> > Friday, December 22, 2017 12:21 AM, Doherty, Declan:
> >> This RFC contains a proposal to add a new tunnel endpoint API to DPDK
> >> that when used in conjunction with rte_flow enables the configuration
> >> of inline data path encapsulation and decapsulation of tunnel
> >> endpoint network overlays on accelerated IO devices.
> >>
> >> The proposed new API would provide for the creation, destruction, and
> >> monitoring of a tunnel endpoint in supporting hw, as well as
> >> capabilities APIs to allow the acceleration features to be discovered by
> applications.
> >>
> 
> >
> >
> > Am not sure I understand why there is a need for the above control
> methods.
> > Are you introducing a new "tep device" ? > As the tunnel endpoint is
> > sending and receiving Ethernet packets from
> the network I think it should still be counted as Ethernet device but with
> more capabilities (for example it supported encap/decap etc..), therefore it
> should use the Ethdev layer API to query statistics (for example).
> 
> No, the new APIs are only intended to be a method of creating, monitoring
> and deleting tunnel-endpoints on an existing ethdev. The rationale for APIs
> separate to rte_flow are the same as that in the rte_security, there is not a
> 1:1 mapping of TEPs to flows. Many flows (VNI's in VxLAN for example) can
> be originate/terminate on the same TEP, therefore managing the TEP
> independently of the flows being transmitted on it is important to allow
> visibility of that endpoint stats for example.

I don't quite understand what you mean by tunnel and flow here. Can you define 
exactly what you mean? Flow is an overloaded word in our world. I think that 
defining it will make understanding the RFC a little easier.

Taking VxLAN, I think of the tunnel as including up through the VxLAN header, 
including the VNI. If you go by this definition, I would consider a flow to be 
all packets with the same VNI and the same 5-tuple hash of the inner packet. Is 
this what you mean by tunnel (or TEP) and flow here?

With these definitions, VPP for example might need up to a couple thousand TEPs 
on an interface and each TEP could have hundreds or thousands of flows. It 
would be quite possible to have 1 rte flow rule per TEP (or 2- ingress/decap 
and egress/encap). The COUNT action could be used to count the number of 
packets through each TEP. Is this adequate, or are you proposing that we need a 
mechanism to get stats of flows within each TEP? Is that the main point of the 
API? Assuming no need for stats on a per TEP/flow basis is there anything else 
the API adds?

> I can't see how the existing
> ethdev API could be used for statistics as a single ethdev could be supporting
> may concurrent TEPs, therefore we would either need to use the extended
> stats with many entries, one for each TEP, or if we treat a TEP as an 
> attribute
> of a port in a similar manner to the way rte_security manages an IPsec SA,
> the state of each TEP can be monitored and managed independently of both
> the overall port or the flows being transported on that endpoint.

Assuming we can define one rte_flow rule per TEP, does what you propose give us 
anything more than just using the COUNT action?
> 
> > As for the capabilities - what specifically you had in mind? The current
> usage you show with tep is with rte_flow rules. There are no capabilities
> currently for rte_flow supported actions/pattern. To check such capabilities
> application uses rte_flow_validate.
> 
> I envisaged that the application should be able to see if an ethdev can
> support TEP in the rx/tx offloads, and then the rte_tep_capabilities would
> allow applications to query what tunnel endpoint protocols are supported
> etc. I would like a simple mechanism to allow users to see if a particular
> tunnel endpoint type is supported without having to build actual flows to
> validate.

I can see the value of that, but in the end wouldn't the API call 
rte_flow_validate anyways? Maybe we don't add the layer now or maybe it doesn't 
really belong in DPDK? I'm in favor of deferring the capabilities API until we 
know it's really needed.  I hate to see special capabilities APIs start 
sneaking in after we decided to go the rte_flow_validate route and users are 
starting to get used to it.
> 
> > Regarding the creation/destroy of tep. Why not simply use rte_flow API
> and avoid this extra control?
> > For example - with 17.11 APIs, application can put the port in isolate mode,
> and insert a flow_rule to catch only IPv4 VXLAN traf

Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation

[John Daley (johndale)]

> -Original Message-
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Monday, September 18, 2017 3:25 PM
> To: John Daley (johndale) 
> Cc: Ferruh Yigit ; dev@dpdk.org; Sergio Gonzalez
> Monroy 
> Subject: Re: [PATCH] net/enic: fix multi-process operation
> 
> 18/09/2017 23:27, Ferruh Yigit:
> > On 9/11/2017 7:58 PM, John Daley wrote:
> > > - Use rte_malloc() instead of malloc() for the per device 'vdev' structure
> > >   so that it can be shared across processes.
> > > - Only initialize the device if the process type is RTE_PROC_PRIMARY
> > > - Only allow the primary process to do queue setup, start/stop, promisc
> > >   allmulticast, mac add/del, mtu.
> [...]
> > > --- a/drivers/net/enic/enic_ethdev.c
> > > +++ b/drivers/net/enic/enic_ethdev.c
> > > @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
> > > *dev,  static void enicpmd_dev_tx_queue_release(void *txq)  {
> > >   ENICPMD_FUNC_TRACE();
> > > +
> > > + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > > + return;
> > > +
> >
> > Hi John,
> >
> > I am not sure about these updates. Agree that these functions should
> > know process type, but all others PMDs don't do this.

I looked at mlx5 and it does something similar with its mlx5_is_secondary() in 
functions that modify fields in its shared private structure.

> >
> > Added a few more people for comment, but as far I understand its
> > application responsibility to NOT call these functions if it is
> > secondary process.
> >
> > For device init/uninit, that is part of eal_init() and have to be
> > called both for primary and secondary process and PMD needs to protect
> > it, for other functions application's responsibility.

I put the checks into the PMD because I didn't want to trust the app and I 
didn't see checks in the library functions. I assumed that is why it was done 
in mlx5. I was afraid that the secondary may try to write fields in the shared 
structure and cause some silent bad behavior, like if secondary sets the MTU 
then the primary does, then secondary assumes it was different than what it is, 
or something like that.
> 
> Yes for now it is the policy.
> But it is a gray area and it could be clearer with my "ownership proposal":
>   http://dpdk.org/ml/archives/dev/2017-September/074656.html
> A secondary process could manage the ports it owns.

I think the ownership concept would help make DPDK more flexible and 
potentially cleaner. Perhaps ownership checks could be pushed the lib 
functions, like rte_eth_dev_set_mtu(), and remove all such checks in the 
PMD(s). 
> 
> Feel free to comment the proposal.

Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation

[John Daley (johndale)]

Hi Ferruh,
-johnd

> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com]
> Sent: Tuesday, September 19, 2017 5:59 AM
> To: John Daley (johndale) ; Thomas Monjalon
> 
> Cc: dev@dpdk.org; Sergio Gonzalez Monroy
> ; Adrien Mazarguil
> ; Nelio Laranjeiro
> ; Ananyev, Konstantin
> 
> Subject: Re: [dpdk-dev] [PATCH] net/enic: fix multi-process operation
> 
> On 9/19/2017 6:31 AM, John Daley (johndale) wrote:
> >
> >> -Original Message-
> >> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> >> Sent: Monday, September 18, 2017 3:25 PM
> >> To: John Daley (johndale) 
> >> Cc: Ferruh Yigit ; dev@dpdk.org; Sergio
> >> Gonzalez Monroy 
> >> Subject: Re: [PATCH] net/enic: fix multi-process operation
> >>
> >> 18/09/2017 23:27, Ferruh Yigit:
> >>> On 9/11/2017 7:58 PM, John Daley wrote:
> >>>> - Use rte_malloc() instead of malloc() for the per device 'vdev'
> structure
> >>>>   so that it can be shared across processes.
> >>>> - Only initialize the device if the process type is
> >>>> RTE_PROC_PRIMARY
> >>>> - Only allow the primary process to do queue setup, start/stop, promisc
> >>>>   allmulticast, mac add/del, mtu.
> >> [...]
> >>>> --- a/drivers/net/enic/enic_ethdev.c
> >>>> +++ b/drivers/net/enic/enic_ethdev.c
> >>>> @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev
> >>>> *dev,  static void enicpmd_dev_tx_queue_release(void *txq)  {
> >>>>  ENICPMD_FUNC_TRACE();
> >>>> +
> >>>> +if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >>>> +return;
> >>>> +
> >>>
> >>> Hi John,
> >>>
> >>> I am not sure about these updates. Agree that these functions should
> >>> know process type, but all others PMDs don't do this.
> >
> > I looked at mlx5 and it does something similar with its mlx5_is_secondary()
> in functions that modify fields in its shared private structure.
> 
> Right, mlx5 also has these checks.
> 
> >
> >>>
> >>> Added a few more people for comment, but as far I understand its
> >>> application responsibility to NOT call these functions if it is
> >>> secondary process.
> >>>
> >>> For device init/uninit, that is part of eal_init() and have to be
> >>> called both for primary and secondary process and PMD needs to
> >>> protect it, for other functions application's responsibility.
> >
> > I put the checks into the PMD because I didn't want to trust the app and I
> didn't see checks in the library functions. I assumed that is why it was done 
> in
> mlx5. I was afraid that the secondary may try to write fields in the shared
> structure and cause some silent bad behavior, like if secondary sets the MTU
> then the primary does, then secondary assumes it was different than what it
> is, or something like that.
> 
> The set values are in the shared memory, so a variable set by secondary will
> be visible to primary, via versa. Of course a synchronization required
> between primary and secondary processes.
> 
> Also why secondary shouldn't be allowed to do some work, like start/stop
> the port?
> 
> I believe this should be application level concern, at worst libraries but not
> drivers.

I don't disagree, but I would need to verify that relaxing the checks would not 
cause catastrophic errors in case the primary and secondary don't synchronize 
their actions to the PMD. This will take some more testing. The patch I 
provided with the checks is safe as is and so if you could accept it for 17.08 
with the promise that I will work on relaxing the checks in the next release, I 
would appreciate it.
> 
> >>
> >> Yes for now it is the policy.
> >> But it is a gray area and it could be clearer with my "ownership proposal":
> >>http://dpdk.org/ml/archives/dev/2017-September/074656.html
> >> A secondary process could manage the ports it owns.
> >
> > I think the ownership concept would help make DPDK more flexible and
> potentially cleaner. Perhaps ownership checks could be pushed the lib
> functions, like rte_eth_dev_set_mtu(), and remove all such checks in the
> PMD(s).
> >>
> >> Feel free to comment the proposal.

Re: [dpdk-dev] [RFC 1/4] enic: update format string to match arg types

[John Daley (johndale)]

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Aaron Conole
> Sent: Tuesday, September 26, 2017 11:53 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC 1/4] enic: update format string to match arg types
> 
> The argument `index` (and unique_id) is unsigned, but the format string type
> used was for signed types.
> 
> Signed-off-by: Aaron Conole 
> ---
>  drivers/net/enic/base/vnic_cq.c  | 4 ++--  drivers/net/enic/base/vnic_dev.c
> | 6 +++---  drivers/net/enic/base/vnic_rq.c  | 4 ++--
> drivers/net/enic/base/vnic_wq.c  | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/enic/base/vnic_cq.c
> b/drivers/net/enic/base/vnic_cq.c index 2f65f35..33db10f 100644
> --- a/drivers/net/enic/base/vnic_cq.c
> +++ b/drivers/net/enic/base/vnic_cq.c
> @@ -65,11 +65,11 @@ int vnic_cq_alloc(struct vnic_dev *vdev, struct
> vnic_cq *cq, unsigned int index,
> 
>   cq->ctrl = vnic_dev_get_res(vdev, RES_TYPE_CQ, index);
>   if (!cq->ctrl) {
> - pr_err("Failed to hook CQ[%d] resource\n", index);
> + pr_err("Failed to hook CQ[%u] resource\n", index);
>   return -EINVAL;
>   }
> 
> - snprintf(res_name, sizeof(res_name), "%d-cq-%d", instance++,
> index);
> + snprintf(res_name, sizeof(res_name), "%d-cq-%u", instance++,
> index);
>   err = vnic_dev_alloc_desc_ring(vdev, &cq->ring, desc_count,
> desc_size,
>   socket_id, res_name);
>   if (err)
> diff --git a/drivers/net/enic/base/vnic_dev.c
> b/drivers/net/enic/base/vnic_dev.c
> index 49b3655..808f95b 100644
> --- a/drivers/net/enic/base/vnic_dev.c
> +++ b/drivers/net/enic/base/vnic_dev.c
> @@ -650,7 +650,7 @@ int vnic_dev_stats_dump(struct vnic_dev *vdev,
> struct vnic_stats **stats)
> 
>   if (!vdev->stats) {
>   snprintf((char *)name, sizeof(name),
> - "vnic_stats-%d", instance++);
> + "vnic_stats-%u", instance++);
>   vdev->stats = vdev->alloc_consistent(vdev->priv,
>   sizeof(struct vnic_stats), &vdev->stats_pa, (u8
> *)name);
>   if (!vdev->stats)
> @@ -900,7 +900,7 @@ int vnic_dev_notify_set(struct vnic_dev *vdev, u16
> intr)
>   }
>   if (!vnic_dev_in_reset(vdev)) {
>   snprintf((char *)name, sizeof(name),
> - "vnic_notify-%d", instance++);
> + "vnic_notify-%u", instance++);
>   notify_addr = vdev->alloc_consistent(vdev->priv,
>   sizeof(struct vnic_devcmd_notify),
>   ¬ify_pa, (u8 *)name);
> @@ -1150,7 +1150,7 @@ int vnic_dev_classifier(struct vnic_dev *vdev, u8
> cmd, u16 *entry,
>   tlv_size = filter_size + action_size +
>   2*sizeof(struct filter_tlv);
>   snprintf((char *)z_name, sizeof(z_name),
> - "vnic_clsf_%d", unique_id++);
> + "vnic_clsf_%u", unique_id++);
>   tlv_va = vdev->alloc_consistent(vdev->priv,
>   tlv_size, &tlv_pa, (u8 *)z_name);
>   if (!tlv_va)
> diff --git a/drivers/net/enic/base/vnic_rq.c
> b/drivers/net/enic/base/vnic_rq.c index 10a40c1..776a6f7 100644
> --- a/drivers/net/enic/base/vnic_rq.c
> +++ b/drivers/net/enic/base/vnic_rq.c
> @@ -58,13 +58,13 @@ int vnic_rq_alloc(struct vnic_dev *vdev, struct vnic_rq
> *rq, unsigned int index,
> 
>   rq->ctrl = vnic_dev_get_res(vdev, RES_TYPE_RQ, index);
>   if (!rq->ctrl) {
> - pr_err("Failed to hook RQ[%d] resource\n", index);
> + pr_err("Failed to hook RQ[%u] resource\n", index);
>   return -EINVAL;
>   }
> 
>   vnic_rq_disable(rq);
> 
> - snprintf(res_name, sizeof(res_name), "%d-rq-%d", instance++,
> index);
> + snprintf(res_name, sizeof(res_name), "%d-rq-%u", instance++,
> index);
>   rc = vnic_dev_alloc_desc_ring(vdev, &rq->ring, desc_count,
> desc_size,
>   rq->socket_id, res_name);
>   return rc;
> diff --git a/drivers/net/enic/base/vnic_wq.c
> b/drivers/net/enic/base/vnic_wq.c index 7c4119c..273e3db 100644
> --- a/drivers/net/enic/base/vnic_wq.c
> +++ b/drivers/net/enic/base/vnic_wq.c
> @@ -52,7 +52,7 @@ int vnic_wq_alloc_ring(struct vnic_dev *vdev, struct
> vnic_wq *wq,
>   char res_name[NAME_MAX];
>   static int instance;
> 
> - snprintf(res_name, sizeof(res_name), "%d-wq-%d", instance++, wq-
> >index);
> + snprintf(res_name, sizeof(res_name), "%d-wq-%u", instance++,
> +wq->index);
>   return vnic_dev_alloc_desc_ring(vdev, &wq->ring, desc_count,
> desc_size,
>   wq->socket_id, res_name);
>  }
> --
> 2.9.5

Reviewed-by: John Daley 

Thanks,
Johnd

Re: [dpdk-dev] [RFC 2/4] enic: fix assignment

[John Daley (johndale)]

Aaron,

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Aaron Conole
> Sent: Tuesday, September 26, 2017 11:53 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC 2/4] enic: fix assignment
> 
> As it stands, the existing assingment to mbuf has no effect outside of the
> function.  Prior to this change, the mbuf argument would contain an invalid
> address, but it would not be null.  After this change, the caller gets a null
> mbuf back.
> 
> Signed-off-by: Aaron Conole 
> ---
>  drivers/net/enic/enic_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 40dbec7..ff8e4c5 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -224,7 +224,7 @@ enic_free_rq_buf(struct rte_mbuf **mbuf)
>   return;
> 
>   rte_pktmbuf_free(*mbuf);
> - mbuf = NULL;
> + *mbuf = NULL;
>  }
> 
>  void enic_init_vnic_resources(struct enic *enic)

As it turns out, this function is only called when the device is stopped and 
restarting the device overwrites the mbuf pointers with newly allocated ones, 
so there is currently no bad behavior. The intent was to NULL out the pointer 
though and it's certainly better form so I agree with the change.

Reviewed-by: John Daley 

Thanks,
Johnd
> --
> 2.9.5

Re: [dpdk-dev] [RFC 3/4] enic: remove unused code

[John Daley (johndale)]

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Aaron Conole
> Sent: Tuesday, September 26, 2017 11:53 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC 3/4] enic: remove unused code
> 
> The functions here aren't called anywhere in code, at least according to both
> the compiler, and some greps.
> 
> Signed-off-by: Aaron Conole 
> ---
>  drivers/net/enic/base/vnic_cq.c  |  10   drivers/net/enic/base/vnic_dev.c
> | 102 ---
>  drivers/net/enic/base/vnic_rq.c  |   5 --
>  drivers/net/enic/base/vnic_rss.c |  32 
>  drivers/net/enic/base/vnic_wq.c  |   5 --
>  drivers/net/enic/enic.h  |   5 --
>  drivers/net/enic/enic_compat.h   |  20 
>  7 files changed, 179 deletions(-)
> 
> diff --git a/drivers/net/enic/base/vnic_cq.c
> b/drivers/net/enic/base/vnic_cq.c index 33db10f..3549fec 100644
> --- a/drivers/net/enic/base/vnic_cq.c
> +++ b/drivers/net/enic/base/vnic_cq.c
> @@ -35,16 +35,6 @@
>  #include "vnic_dev.h"
>  #include "vnic_cq.h"
> 
> -int vnic_cq_mem_size(struct vnic_cq *cq, unsigned int desc_count,
> - unsigned int desc_size)
> -{
> - int mem_size;
> -
> - mem_size = vnic_dev_desc_ring_size(&cq->ring, desc_count,
> desc_size);
> -
> - return mem_size;
> -}
> -
>  void vnic_cq_free(struct vnic_cq *cq)
>  {
>   vnic_dev_free_desc_ring(cq->vdev, &cq->ring); diff --git
> a/drivers/net/enic/base/vnic_dev.c b/drivers/net/enic/base/vnic_dev.c
> index 808f95b..a4b7f4b 100644
> --- a/drivers/net/enic/base/vnic_dev.c
> +++ b/drivers/net/enic/base/vnic_dev.c
> @@ -443,24 +443,6 @@ static int vnic_dev_cmd_no_proxy(struct vnic_dev
> *vdev,
>   return err;
>  }
> 
> -void vnic_dev_cmd_proxy_by_index_start(struct vnic_dev *vdev, u16
> index) -{
> - vdev->proxy = PROXY_BY_INDEX;
> - vdev->proxy_index = index;
> -}
> -
> -void vnic_dev_cmd_proxy_by_bdf_start(struct vnic_dev *vdev, u16 bdf) -{
> - vdev->proxy = PROXY_BY_BDF;
> - vdev->proxy_index = bdf;
> -}
> -
> -void vnic_dev_cmd_proxy_end(struct vnic_dev *vdev) -{
> - vdev->proxy = PROXY_NONE;
> - vdev->proxy_index = 0;
> -}
> -
>  int vnic_dev_cmd(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
>   u64 *a0, u64 *a1, int wait)
>  {
> @@ -672,15 +654,6 @@ int vnic_dev_close(struct vnic_dev *vdev)
>   return vnic_dev_cmd(vdev, CMD_CLOSE, &a0, &a1, wait);  }
> 
> -/** Deprecated.  @see vnic_dev_enable_wait */ -int
> vnic_dev_enable(struct vnic_dev *vdev) -{
> - u64 a0 = 0, a1 = 0;
> - int wait = 1000;
> -
> - return vnic_dev_cmd(vdev, CMD_ENABLE, &a0, &a1, wait);
> -}
> -
>  int vnic_dev_enable_wait(struct vnic_dev *vdev)  {
>   u64 a0 = 0, a1 = 0;
> @@ -725,31 +698,6 @@ int vnic_dev_open_done(struct vnic_dev *vdev, int
> *done)
>   return 0;
>  }
> 
> -int vnic_dev_soft_reset(struct vnic_dev *vdev, int arg) -{
> - u64 a0 = (u32)arg, a1 = 0;
> - int wait = 1000;
> -
> - return vnic_dev_cmd(vdev, CMD_SOFT_RESET, &a0, &a1, wait);
> -}
> -
> -int vnic_dev_soft_reset_done(struct vnic_dev *vdev, int *done) -{
> - u64 a0 = 0, a1 = 0;
> - int wait = 1000;
> - int err;
> -
> - *done = 0;
> -
> - err = vnic_dev_cmd(vdev, CMD_SOFT_RESET_STATUS, &a0, &a1,
> wait);
> - if (err)
> - return err;
> -
> - *done = (a0 == 0);
> -
> - return 0;
> -}
> -
>  int vnic_dev_get_mac_addr(struct vnic_dev *vdev, u8 *mac_addr)  {
>   u64 a0 = 0, a1 = 0;
> @@ -840,19 +788,6 @@ int vnic_dev_set_ig_vlan_rewrite_mode(struct
> vnic_dev *vdev,
>   return 0;
>  }
> 
> -int vnic_dev_raise_intr(struct vnic_dev *vdev, u16 intr) -{
> - u64 a0 = intr, a1 = 0;
> - int wait = 1000;
> - int err;
> -
> - err = vnic_dev_cmd(vdev, CMD_IAR, &a0, &a1, wait);
> - if (err)
> - pr_err("Failed to raise INTR[%d], err %d\n", intr, err);
> -
> - return err;
> -}
> -
>  void vnic_dev_set_reset_flag(struct vnic_dev *vdev, int state)  {
>   vdev->in_reset = state;
> @@ -985,14 +920,6 @@ int vnic_dev_init(struct vnic_dev *vdev, int arg)
>   return r;
>  }
> 
> -int vnic_dev_deinit(struct vnic_dev *vdev) -{
> - u64 a0 = 0, a1 = 0;
> - int wait = 1000;
> -
> - return vnic_dev_cmd(vdev, CMD_DEINIT, &a0, &a1, wait);
> -}
> -
>  void vnic_dev_intr_coal_timer_info_default(struct vnic_dev *vdev)  {
>   /* Default: hardware intr coal timer is in units of 1.5 usecs */ @@ -
> 1018,18 +945,6 @@ u32 vnic_dev_port_speed(struct vnic_dev *vdev)
>   return vdev->notify_copy.port_speed;
>  }
> 
> -void vnic_dev_set_intr_mode(struct vnic_dev *vdev,
> - enum vnic_dev_intr_mode intr_mode)
> -{
> - vdev->intr_mode = intr_mode;
> -}
> -
> -enum vnic_dev_intr_mode vnic_dev_get_intr_mode(
> - struct vnic_dev *vdev)
> -{
> - return vdev->intr_mode;
> -}
> -
>  u32 vnic_dev_intr_coal_timer_usec_to_hw(struct vnic_dev *vdev, u32
> usec)  {
>   return (usec * vdev->intr_coal_timer_info.mul) / @@ -1094,23
> +1009,6 @@ struct vn

Re: [dpdk-dev] [RFC 4/4] enic: remove anscillary assignment

[John Daley (johndale)]

> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Aaron Conole
> Sent: Tuesday, September 26, 2017 11:53 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC 4/4] enic: remove anscillary assignment
> 
> The assignment at initialization is overwritten immediately.  Drop the
> assignment.
> 
> Signed-off-by: Aaron Conole 
> ---
>  drivers/net/enic/base/vnic_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/enic/base/vnic_dev.c
> b/drivers/net/enic/base/vnic_dev.c
> index a4b7f4b..932667f 100644
> --- a/drivers/net/enic/base/vnic_dev.c
> +++ b/drivers/net/enic/base/vnic_dev.c
> @@ -272,7 +272,7 @@ int vnic_dev_alloc_desc_ring(struct vnic_dev *vdev,
>   __attribute__((unused)) unsigned int socket_id,
>   char *z_name)
>  {
> - void *alloc_addr = NULL;
> + void *alloc_addr;
>   dma_addr_t alloc_pa = 0;
> 
>   vnic_dev_desc_ring_size(ring, desc_count, desc_size);
> --
> 2.9.5

Reviewed-by: John Daley 

Thanks,
Johnd

Re: [dpdk-dev] [PATCH 37/41] net/enic: use contiguous allocation for DMA memory

[John Daley (johndale)]

Hi Anatoly,
Looks good, see inline for details.
Acked-by: John Daley 

Thanks,
John

> -Original Message-
> From: Anatoly Burakov [mailto:anatoly.bura...@intel.com]
> Sent: Saturday, March 03, 2018 5:46 AM
> To: dev@dpdk.org
> Cc: John Daley (johndale) ; Hyong Youb Kim (hyonkim)
> ; keith.wi...@intel.com; jianfeng@intel.com;
> andras.kov...@ericsson.com; laszlo.vadk...@ericsson.com;
> benjamin.wal...@intel.com; bruce.richard...@intel.com;
> tho...@monjalon.net; konstantin.anan...@intel.com;
> kuralamudhan.ramakrish...@intel.com; louise.m.d...@intel.com;
> nelio.laranje...@6wind.com; ys...@mellanox.com; peppe...@japf.ch;
> jerin.ja...@caviumnetworks.com; hemant.agra...@nxp.com;
> olivier.m...@6wind.com
> Subject: [PATCH 37/41] net/enic: use contiguous allocation for DMA memory
> 
> Signed-off-by: Anatoly Burakov 
> ---
> 
> Notes:
> It is not 100% clear that second call to memzone_reserve
> is allocating DMA memory. Corrections welcome.
The 2nd call is allocating DMA memory so I believe your patch is correct.
> 
>  drivers/net/enic/enic_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c index
> ec9d343..cb2a7ba 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -319,7 +319,7 @@ enic_alloc_consistent(void *priv, size_t size,
>   struct enic *enic = (struct enic *)priv;
>   struct enic_memzone_entry *mze;
> 
> - rz = rte_memzone_reserve_aligned((const char *)name,
> + rz = rte_memzone_reserve_aligned_contig((const char *)name,
>size, SOCKET_ID_ANY, 0,
> ENIC_ALIGN);
>   if (!rz) {
>   pr_err("%s : Failed to allocate memory requested for %s\n",
> @@ -787,7 +787,7 @@ int enic_alloc_wq(struct enic *enic, uint16_t queue_idx,
>"vnic_cqmsg-%s-%d-%d", enic->bdf_name, queue_idx,
>   instance++);
> 
> - wq->cqmsg_rz = rte_memzone_reserve_aligned((const char *)name,
> + wq->cqmsg_rz = rte_memzone_reserve_aligned_contig((const char
> *)name,
>  sizeof(uint32_t),
>  SOCKET_ID_ANY, 0,
>  ENIC_ALIGN);
This is a send completion landing spot which is DMA'd to by the NIC so it does 
have to be contiguous. However the size is only 4 bytes so it might not matter.
> --
> 2.7.4

[dpdk-dev] enic in passhtrough mode tx drops

[John Daley (johndale)]

Hi Ruth,
I'm the enic pmd maintainer. To cut down on chatter I can work with you off 
list and then we can post the result to dev at .
I'd like to see the packet formed in the app and what's on the wire egress, 
that you are sending on a configured VLAN, etc.
I'll contact you directly.
-john

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ruth Christen
> Sent: Thursday, June 16, 2016 6:13 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] enic in passhtrough mode tx drops
> 
> Hi all,
> 
> I'm running a vm attached to 2 cisco Virtual Card Interfaces in passthrough
> mode in a cisco UCS. The vNICs are configured on access mode without VLAN
> ID.
> 
> The incoming packets are arriving with 802.1q header containing vlan priority
> bit according to the class of service configured on the vNIC. I understood 
> this
> is expected from a fiber channel Ethernet card.
> 
> According to dpdk documentation there's a need to set the
> VLAN_STRIP_OFFLOAD flag and call rte_eth_dev_set_vlan_offload on the
> ports.
> 
> If I run a simple l2fwd application where the same packet received in one
> port is sent through the other the traffic works ok.
> 
> If I generate the packets in my vm and send them out traffic doesn't work. (I
> tried send the traffic out with/without a 802.1q header with priority bit)
> 
> 
> 
> Is there a specific configuration to be added to the mbuff for the tx packets
> generated in the VM? Could be the vlan_tci/ ol_flags/ or any other missing
> flag set?
> 
> Does somebody know the exact behavior of the enic card with the priority
> tagging?
> 
> 
> 
> BTW in virtio mode the traffic works in both the flows.
> 
> 
> 
> Thanks a lot!
> 
> 

[dpdk-dev] unchecked return value in enic driver

[John Daley (johndale)]

> -Original Message-
> From: Kerlin, MarcinX [mailto:marcinx.kerlin at intel.com]
> Sent: Monday, June 20, 2016 4:12 AM
> To: John Daley (johndale) ; Nelson Escobar
> (neescoba) 
> Cc: 'dev at dpdk.org' 
> Subject: RE: unchecked return value in enic driver
> 
> Hi John and Nelson,
> 
> > -Original Message-
> > From: Kerlin, MarcinX
> > Sent: Monday, June 13, 2016 1:18 PM
> > To: johndale at cisco.com; neescoba at cisco.com
> > Cc: dev at dpdk.org
> > Subject: unchecked return value in enic driver
> >
> > Hi John and Nelson,
> >
> > I have a question regarding Coverity defects:
> >
> > File: /drivers/net/enic/enic_ethdev.c
> > Line: 379
> >
> > CID 13197: Unchecked return value
> > (CHECKED_RETURN)1.?check_return:?Calling?rte_atomic64_cmpset?without
> > checking return value (as is done elsewhere 15 out of 17 times)
> >
> > Can I mark this error as "False Positive" in Coverity Classification ? 
> > reason:
> > 1. Function returns a void type so change the return type to int
> > requires changes all drivers 2. rte_atomic64_cmpset is at the end of
> > function so nonsense added a return
> >
> > What is your opinion?

I agree with marking it false positive for the reason you mention. 
Thanks!
John

> 
> I marked this Coverity as false-positive with an explanation. If in your 
> opinion
> it is not ok, you can reopen/change/fix it.
> 
> >
> > Regards,
> > Marcin

[dpdk-dev] [PATCH 2/4] enic: set the max allowed MTU for the NIC

[John Daley (johndale)]

Hi Bruce,

> > * What was the MTU set to by default before this patch is applied? Was
> > it just set to 1518 or something else?
> > * What happens, if anything, if buffers bigger than the MTU size are sent
> down?
> This is obviously referring to buffers bigger than MTU on TX. There is also 
> the
> question of what happens if buffer sizes smaller than MTU are provided on
> RX.

I think I answered all your questions in the revised commit messages of the v2 
patchset (and then some) except this last one. Enic doesn't do any checking on 
Rx that buffers are greater than the MTU since it would affect performance. 
However if a packet is bigger than a buffer and Rx scatter is disabled, the 
packet will be dropped and 'imissed' incremented.

Thanks,
Johnd

[dpdk-dev] [PATCH] net/enic: remove useless assert macro

[John Daley (johndale)]

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, July 06, 2016 8:25 AM
> To: John Daley (johndale) ; Nelson Escobar
> (neescoba) 
> Cc: dev at dpdk.org
> Subject: [PATCH] net/enic: remove useless assert macro
> 
> The macro ENIC_ASSERT does the same thing as RTE_ASSERT, thus it can be
> removed.
> 
> Signed-off-by: Thomas Monjalon 
Acked-by: John Daley 

[dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference count before recycling

[John Daley (johndale)]

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Monday, July 11, 2016 3:04 AM
> To: John Daley (johndale) ; dev at dpdk.org
> Cc: bruce.richardson at intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/enic: decrement Tx mbuf reference
> count before recycling
> 
> Hi John,
> 
> On 07/09/2016 12:22 AM, John Daley wrote:
> > In the Tx cleanup function, the reference count in mbufs to be
> > returned to the pool should to be decremented before they are
> > returned. Decrementing is not done by rte_mempool_put_bulk() so it
> > must be done separately using __rte_pktmbuf_prefree_seg().
> > If decrementing does not result in a 0 reference count the mbuf is not
> > returned to the pool and whatever has the last reference is
> > responsible for freeing.
> >
> > Fixes: 36935afbc53c ("net/enic: refactor Tx mbuf recycling")
> > Reviewed-by: Nelson Escobar 
> > Signed-off-by: John Daley 
> > ---
> > Since reference counts are set to 0 when mbufs are reallocated from
> > the pool, and sending packets with reference count not equal to 1 is
> > probably an application error, this patch may not be critical. But a
> > debug ASSERT caught it and it would be nice to have it fixed in 16.07.
> 
> Sending a packet with refcnt != 1 is not an error. It can happen when using
> mbuf clones. So indeed it would be better to have in 16.07.
> 
> For the same reason, I also wonder if enic_free_wq_buf() should also be
> updated with:
> 
> -   rte_mempool_put(mbuf->pool, mbuf);
> +   rte_pktmbuf_free(mbuf);
That is a very good point, thank you. I'll use rte_pktmubf_free_seg(mbuf) 
though, since we are walking an array of all mbuf segments. V2 coming 
momentarily.
-john
> 
> 
> Regards,
> Olivier

[dpdk-dev] [PATCH v2] doc: announce ABI change for mbuf structure

[John Daley (johndale)]

> 
> For 16.11, the mbuf structure will be modified implying ABI breakage.
> Some discussions already took place here:
> http://www.dpdk.org/dev/patchwork/patch/12878/
> 
> Signed-off-by: Olivier Matz 
> ---

Acked-by: John Daley 

Also, definitely +1 on trying to get m->next into the first cache line.

[dpdk-dev] [PATCH v3 07/13] enic: use Tx completion messages instead of descriptors

[John Daley (johndale)]

> -Original Message-
> From: Bruce Richardson [mailto:bruce.richardson at intel.com]
> Sent: Friday, June 10, 2016 2:18 PM
> To: John Daley (johndale) 
> Cc: dev at dpdk.org; bruce.richarsdon at intel.com
> Subject: Re: [dpdk-dev] [PATCH v3 07/13] enic: use Tx completion messages
> instead of descriptors
> 
> On Thu, Jun 02, 2016 at 05:22:51PM -0700, John Daley wrote:
> > The NIC can either DMA a separate completion message for each
> > completed send or periodically just DMA an index of the last completed
> > send. Switch to the second method which improves cache locality and
> > performance.
> >
> > Signed-off-by: John Daley 
> 
> Can you perhaps send me an updated wording for this commit message as
> the title and commit message conflict. The title says to use completion
> messages not descriptors, while the body talks about moving away from a
> completion message way of working.
> Is the former method a descriptor writeback method, while the latter a head
> pointer writeback? If so, I think the title could be:
> 
> "enic: use Tx head pointer not descriptor writeback"
> 
> or something similar.
> 
> Again, if you send on the updated commit text, I'll just update it on apply. 
> I'd
> ideally like to get this patchset pushed to next-net first thing Monday.

Ok, I agree that it is confusing.
We moved from having the hardware send a completion descriptor for every packet 
to having it send the index of the last completed packet every once in a while. 
We can use the word 'index' and 'message' to describe the 2 methods and drop 
the word 'descriptor'. Here is a suggestion:

enic: use Tx completion index instead of completion messages

The NIC can either DMA a separate completion message for each completed send or 
periodically just DMA an index of the last completed send. Switch to the latter 
method which improves cache locality and performance.

Thank you,
John
> 
> /Bruce

[dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled

[John Daley (johndale)]

Hi,

When VLAN stripping is disabled, X710 and 82599ES act differently for me in 
this case when receiving VLAN tagged packets. On 82599ES the flag is set but on 
X710 the flag not set.

Do I maybe have old X710 firmware? Or is it not set for X710 on purpose in this 
case and instead the flag is used to indicate if vlan_tci is valid? Right now 
the enic pmd does what my X710 does, which I think is incorrect and I want to 
fix it.

Thanks,
John

[dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled

[John Daley (johndale)]

Hi Olivier and Ananyev,

I like the new packet types and how they work the same for VLAN and QINQ. Just 
so I understand your suggestion, X710 (as it seems to work today) would not set 
RTE_PTYPE_L2_ETHER_VLAN  in dev_supported_ptypes_get() because it does not know 
how to determine that packet type in the receive path if stripping is disabled? 
But if stripping was enabled, the application could still trust m->vlan_tci if 
the flag was set?

Re changing the meaning of the PKT_RX_VLAN_PKT flag- I think it could cause 
hard to find errors and confusion. I would rather see the flag deprecated and a 
new one defined. Perhaps the flag could be called PKT_RX_VLAN_STRIPPED*.

Maybe another less elegant but more compatible solution would be just keep the 
Niantic behavior and fix other pmd's to match its behavior. For X710, with vlan 
stripping disabled this might mean looking at each packet's Ethernet type and 
set the flag accordingly.  It might not be too expensive since Ethernet type is 
in the 1st cacheline and hopefully prefetched. Thoughts?

*In the future perhaps another flag could be added called 
PKT_RX_VLAN_TCI_VALID. This may not be the same as PKT_RX_VLAN_STRIPPED- enic 
and maybe some other nics are able to set vlan_tci even when not stripping vlan 
tags and this feature could be exposed with this separate flag.

-john

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Monday, April 25, 2016 6:51 AM
> To: Ananyev, Konstantin ; John Daley
> (johndale) ; dev at dpdk.org
> Subject: Re: [dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled
> 
> Hi,
> 
> On 04/25/2016 02:02 PM, Ananyev, Konstantin wrote:
> > Hi John,
> > From rte_mbuf.h:
> > #define PKT_RX_VLAN_PKT  (1ULL << 0)  /**< RX packet is a 802.1q VLAN
> packet. */
> > So yes, in theory it should be set up for vlan packet with both stripping
> on/off.
> > The problem is that (as far as I know) when VLAN stripping is disabled
> > FVL RXD doesn't contain information does that packet contain a VLAN or
> not.
> > Don't really know what is the best option in that case: keep things as
> > it is or change the meaning of the VLAN_PKT flag to indicate is
> mbuf.vlan_tci field is valid or not.
> > Konstantin
> 
> It seems the meaning of the PKT_RX_VLAN_PKT bit depends on the port
> configuration:
> - if vlan stripping is configured, it means VLAN is present in vlan_tci
>   mbuf field
> - if not configured, it means a VLAN is present in the packet
> 
> I don't think this is a good behavior since the application has to know the 
> port
> configuration to properly interpret the meaning of the flag.
> 
> I suggest to change the meaning of this flag to: "vlan was stripped by
> hardware, and vlan tag is now located in m->vlan_tci".
> 
> The same could apply to PKT_RX_QINQ_PKT and m->outer_vlan_tci.
> 
> We could add a new packet_type to tell if the mbuf is a VLAN/QinQ is
> detected in the packet but not stripped.
> 
> Example:
> 
> - vlan stripping is disabled
> 
>   - vlan packet recvd: flags=0, ptype=RTE_PTYPE_L2_ETHER_VLAN
>   - qinq packet recvd: flags=0, ptype=RTE_PTYPE_L2_ETHER_QINQ
> 
> - vlan stripping is enabled
> 
>   - vlan packet recvd: flags=PKT_RX_VLAN_PKT,
> ptype=RTE_PTYPE_L2_ETHER,
> m->vlan_tci=id
>   - qinq packet recvd: flags=PKT_RX_VLAN_PKT|PKT_RX_QINQ_PKT,
> ptype=RTE_PTYPE_L2_ETHER, m->vlan_tci=id, m->vlan_tci_outer=id
> 
> 
> Thoughts?
> 
> 
> 
> >
> >> -Original Message-
> >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Daley
> >> (johndale)
> >> Sent: Friday, April 22, 2016 12:37 AM
> >> To: dev at dpdk.org
> >> Subject: [dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled
> >>
> >> Hi,
> >>
> >> When VLAN stripping is disabled, X710 and 82599ES act differently for
> >> me in this case when receiving VLAN tagged packets. On 82599ES the flag
> is set but on X710 the flag not set.
> >>
> >> Do I maybe have old X710 firmware? Or is it not set for X710 on
> >> purpose in this case and instead the flag is used to indicate if vlan_tci 
> >> is
> valid? Right now the enic pmd does what my X710 does, which I think is
> incorrect and I want to fix it.
> >>
> >> Thanks,
> >> John
> >

[dpdk-dev] removing mbuf error flags

[John Daley (johndale)]

Hi,

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Friday, April 29, 2016 5:25 AM
> To: dev at dpdk.org; Zhang, Helin 
> Cc: Ananyev, Konstantin ; John Daley
> (johndale) 
> Subject: removing mbuf error flags
> 
> Hi,
> 
> In rte_mbuf.h, some rx flags are set to 0 since a long time since nearly 2
> years. It means nobody use them. They were introduced by the following
> commit:
> 
>   http://dpdk.org/browse/dpdk/commit/?id=c22265f6
> 
> As far as I understand, these flags were introduced to let the application
> know that a received packet is invalid.
> 
> The 2 drivers using them are i40e and enic. But as this flags are 0 today, it
> means that invalid packets are silently given to the application.
> 
> My opinion is that invalid packets should not be given to the application and
> only a statistic counter should be incremented.
> No application check these flags today (in examples, or testpmd).
> 
> I would like to remove these flags.
> Thoughs?

I agree. Enic needs a little work to increment a counter and update internal 
indexes correctly. If you are in a hurry, feel free to 's/PKT_RX_MAC_ERR/0/' in 
enic for now.

-John

Re: [dpdk-dev] [PATCH v2] net/enic: add private API to set ingress VLAN rewrite mode

[John Daley (johndale)]

Due to time zone differences, I'll answer for Hyong (below).
-john

> -Original Message-
> From: Thomas Monjalon 
> Sent: Wednesday, March 13, 2019 1:36 PM
> To: Ferruh Yigit ; Hyong Youb Kim (hyonkim)
> 
> Cc: Andrew Rybchenko ; Qi Zhang
> ; dev@dpdk.org; John Daley (johndale)
> ; Shahaf Shuler ; Jerin Jacob
> ; David Marchand
> ; Maxime Coquelin
> ; Konstantin Ananyev
> ; Hemant Agrawal
> ; Stephen Hemminger
> 
> Subject: Re: [PATCH v2] net/enic: add private API to set ingress VLAN rewrite
> mode
> 
> 13/03/2019 19:32, Ferruh Yigit:
> > On 3/5/2019 7:11 AM, Hyong Youb Kim wrote:
> > > The driver currently has a devarg to set the rewrite mode during
> > > init. Some apps want to programatically set it after running
> > > rte_eal_init() and finding that ports are VIC. Add a private
> > > function to support such applications.
> >
> > It is not good idea to have PMD specific APIs (although we already have
> some).
> >
> > Specific to this case, as far as I can see it is to pass a config
> > value and do the action related to it, what would you think having a
> > generic key/value set/get API in ethdev for this? Similar to rawdev
> get_attr/set_attr [1]?
> >
> > My concern is it may turn into something like ioctl with many things
> > pushed to it, and cause possible duplication ...
> 
> Yes, it is clearly ioctl style.
> 
> Please could you explain more what is the rewrite mode?
> Does it apply to the port or the queue?
> 
It applies to a port. By default the Cisco VIC VLAN tags every packet on 
ingress even if they were untagged coming in on the wire. They are tagged with 
VLAN 0 or a VLAN id programmed into the NIC depending on the configuration. Its 
part of the original design, to maintain priority bits, ancient history.

Some apps don't like this (VPP) or take a slower path (OVS). Hyong added a 
ig-vlan-rewrite=untag devarg to disable this (leave untagged/default vlan 
packets untagged) during rte_eal_init and this is helpful for OVS, but VPP 
likes to set the rewrite mode after rte_eal_init() and finding the ports are 
VIC ports. So that is the reasoning behind the private API call.

Re: [dpdk-dev] [PATCH] net/enic: move macro to the correct file

[John Daley (johndale)]

You are right, just need to cast #define parameters, then can use the RTE_MIN 
and MAX.
Will do a patch.
Thanks,
john

> -Original Message-
> From: Thomas Monjalon 
> Sent: Sunday, January 19, 2020 12:24 PM
> To: John Daley (johndale) ; Hyong Youb Kim
> (hyonkim) 
> Cc: ferruh.yi...@intel.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/enic: move macro to the correct file
> 
> 14/01/2020 01:24, John Daley:
> > +#define min_t(type, x, y) ({\
> > +   type __min1 = (x);  \
> > +   type __min2 = (y);  \
> > +   __min1 < __min2 ? __min1 : __min2; })
> > +
> > +#define max_t(type, x, y) ({\
> > +   type __max1 = (x);  \
> > +   type __max2 = (y);  \
> > +   __max1 > __max2 ? __max1 : __max2; })
> 
> Why not using RTE_MIN/RTE_MAX which use typeof?
> You need to specify a type?
> 

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

Ok, lets NAK this patch. See comment inline.
Thanks,
John

> -Original Message-
> From: Ferruh Yigit 
> Sent: Thursday, July 25, 2019 3:07 AM
> To: John Daley (johndale) 
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/enic: retain previous message logging
> 
> On 7/25/2019 3:46 AM, John Daley wrote:
> > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> > default. After the fix, using dynamic logging, only NOTICE level and
> > higher were displayed by default and INFO level were not. Change the
> > messages to NOTICE level so they continue to display.
> >
> > DTS uses testpmd and parses messages and some tests failed because
> > messages were no longer displayed. Other apps may also depend on the
> > messages.
> 
> If you need messages for the test framework, why not just increase the log
> level for enic PMD via application parameter [1], or as command to
> testpmd[2]?
> Since it is dynamic debug now, you don't need to change the default, can
> change the level on demand.

I have no problem modifying our test scripts. The bigger concern was about any 
other scripts out there that might break because the default enic PMD messages 
changed. I suppose chances are slim and any such scripts can easily be modified 
to set the log level to info.

> 
> [1]
> starting testpmd with following option should do it:
> --log-level=pmd.net.enic.*:info
> 
> testpmd --log-level=pmd.net.enic.*:info -- -i
> 
> 
> [2]
> after testpmd started, can change the debug level:
> testpmd> set log pmd.net.enic 7
> 
> 
> [3] bonus, see current log levels
> testpmd> dump_log_types

Nice! I didn't know about this.

> 
> 
> 
> >
> > Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> >
> > Signed-off-by: John Daley 
> 
> <...>

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

Actually, after talking to a couple internal folks, we'd like to get the patch 
in if possible- many of our customer issues are due to the wrong number of 
queues, etc, which are reported in the default logs. To ask them to add 
--log-level=enic,info would be a pain, esp. for apps like OVS, fd.io.
-john

> -Original Message-
> From: John Daley (johndale)
> Sent: Thursday, July 25, 2019 1:26 PM
> To: Ferruh Yigit 
> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) 
> Subject: RE: [PATCH] net/enic: retain previous message logging
> 
> Ok, lets NAK this patch. See comment inline.
> Thanks,
> John
> 
> > -Original Message-
> > From: Ferruh Yigit 
> > Sent: Thursday, July 25, 2019 3:07 AM
> > To: John Daley (johndale) 
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH] net/enic: retain previous message logging
> >
> > On 7/25/2019 3:46 AM, John Daley wrote:
> > > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> > > default. After the fix, using dynamic logging, only NOTICE level and
> > > higher were displayed by default and INFO level were not. Change the
> > > messages to NOTICE level so they continue to display.
> > >
> > > DTS uses testpmd and parses messages and some tests failed because
> > > messages were no longer displayed. Other apps may also depend on the
> > > messages.
> >
> > If you need messages for the test framework, why not just increase the
> > log level for enic PMD via application parameter [1], or as command to
> > testpmd[2]?
> > Since it is dynamic debug now, you don't need to change the default,
> > can change the level on demand.
> 
> I have no problem modifying our test scripts. The bigger concern was about
> any other scripts out there that might break because the default enic PMD
> messages changed. I suppose chances are slim and any such scripts can
> easily be modified to set the log level to info.
> 
> >
> > [1]
> > starting testpmd with following option should do it:
> > --log-level=pmd.net.enic.*:info
> >
> > testpmd --log-level=pmd.net.enic.*:info -- -i
> >
> >
> > [2]
> > after testpmd started, can change the debug level:
> > testpmd> set log pmd.net.enic 7
> >
> >
> > [3] bonus, see current log levels
> > testpmd> dump_log_types
> 
> Nice! I didn't know about this.
> 
> >
> >
> >
> > >
> > > Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> > >
> > > Signed-off-by: John Daley 
> >
> > <...>

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

> -Original Message-
> From: Ferruh Yigit 
> Sent: Friday, July 26, 2019 3:01 AM
> To: John Daley (johndale) 
> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) 
> Subject: Re: [PATCH] net/enic: retain previous message logging
> 
> On 7/26/2019 9:17 AM, John Daley (johndale) wrote:
> > Actually, after talking to a couple internal folks, we'd like to get the 
> > patch in
> if possible- many of our customer issues are due to the wrong number of
> queues, etc, which are reported in the default logs. To ask them to add --log-
> level=enic,info would be a pain, esp. for apps like OVS, fd.io.
> 
> As I said to Hyong, I believe it is not good approach to have logs to debug
> customer issues enabled by default.

Yes, we need to migrate away from this, but to suddenly hide messages that have 
been there all along is worse. We definitely need to work towards less verbose 
default messaging.
> 
> But I see you want to keep the your logging same, instead of replacing all
> 'dev_info' with 'dev_notice', what about setting default log level for driver 
> to
> 'RTE_LOG_INFO', this is easier change with same affect?
> 
> And when more fine grained update done on which logs to really set to
> 'dev_notice', the default log level can be updated back to 'RTE_LOG_NOTICE'

I agree this is a better way to go. I have a V2 patch coming which sets the 
enic PMD default log level to RTE_LOG_INFO. We can submit fine grained updates 
in future commits.
Thanks, John.
>
> > -john
> >
> >> -Original Message-
> >> From: John Daley (johndale)
> >> Sent: Thursday, July 25, 2019 1:26 PM
> >> To: Ferruh Yigit 
> >> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) 
> >> Subject: RE: [PATCH] net/enic: retain previous message logging
> >>
> >> Ok, lets NAK this patch. See comment inline.
> >> Thanks,
> >> John
> >>
> >>> -Original Message-
> >>> From: Ferruh Yigit 
> >>> Sent: Thursday, July 25, 2019 3:07 AM
> >>> To: John Daley (johndale) 
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [PATCH] net/enic: retain previous message logging
> >>>
> >>> On 7/25/2019 3:46 AM, John Daley wrote:
> >>>> Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> >>>> default. After the fix, using dynamic logging, only NOTICE level
> >>>> and higher were displayed by default and INFO level were not.
> >>>> Change the messages to NOTICE level so they continue to display.
> >>>>
> >>>> DTS uses testpmd and parses messages and some tests failed because
> >>>> messages were no longer displayed. Other apps may also depend on
> >>>> the messages.
> >>>
> >>> If you need messages for the test framework, why not just increase
> >>> the log level for enic PMD via application parameter [1], or as
> >>> command to testpmd[2]?
> >>> Since it is dynamic debug now, you don't need to change the default,
> >>> can change the level on demand.
> >>
> >> I have no problem modifying our test scripts. The bigger concern was
> >> about any other scripts out there that might break because the
> >> default enic PMD messages changed. I suppose chances are slim and any
> >> such scripts can easily be modified to set the log level to info.
> >>
> >>>
> >>> [1]
> >>> starting testpmd with following option should do it:
> >>> --log-level=pmd.net.enic.*:info
> >>>
> >>> testpmd --log-level=pmd.net.enic.*:info -- -i
> >>>
> >>>
> >>> [2]
> >>> after testpmd started, can change the debug level:
> >>> testpmd> set log pmd.net.enic 7
> >>>
> >>>
> >>> [3] bonus, see current log levels
> >>> testpmd> dump_log_types
> >>
> >> Nice! I didn't know about this.
> >>
> >>>
> >>>
> >>>
> >>>>
> >>>> Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> >>>>
> >>>> Signed-off-by: John Daley 
> >>>
> >>> <...>
> >

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

> -Original Message-
> From: Stephen Hemminger 
> Sent: Friday, July 26, 2019 1:51 PM
> To: Hyong Youb Kim (hyonkim) 
> Cc: John Daley (johndale) ; Ferruh Yigit
> ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging
> 
> On Fri, 26 Jul 2019 04:21:23 +
> "Hyong Youb Kim (hyonkim)"  wrote:
> 
> > > -Original Message-
> > > From: John Daley (johndale)
> > > Sent: Friday, July 26, 2019 5:26 AM
> > > To: Ferruh Yigit 
> > > Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) 
> > > Subject: RE: [PATCH] net/enic: retain previous message logging
> > >
> > > Ok, lets NAK this patch. See comment inline.
> > > Thanks,
> > > John
> > >
> > [...]
> > > > On 7/25/2019 3:46 AM, John Daley wrote:
> > > > > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd
> > > > > by default. After the fix, using dynamic logging, only NOTICE
> > > > > level and higher were displayed by default and INFO level were
> > > > > not. Change the messages to NOTICE level so they continue to display.
> > > > >
> > > > > DTS uses testpmd and parses messages and some tests failed
> > > > > because messages were no longer displayed. Other apps may also
> > > > > depend on the messages.
> > > >
> > > > If you need messages for the test framework, why not just increase
> > > > the log level for enic PMD via application parameter [1], or as
> > > > command to testpmd[2]?
> > > > Since it is dynamic debug now, you don't need to change the
> > > > default, can change the level on demand.
> > >
> > > I have no problem modifying our test scripts. The bigger concern was
> > > about any other scripts out there that might break because the
> > > default enic PMD messages changed. I suppose chances are slim and
> > > any such scripts can easily be modified to set the log level to info.
> > >
> >
> > Hi John, Ferruh,
> >
> > Can you guys reconsider? John's commit message makes it sound like he
> > is modifying PMD to avoid modifying test scripts. That is not the
> > issue at all. The real problem is that his previous commit causes a
> > customer visible change, which can lead to a lot of headache for both
> > us (doing tech support) and customers (wondering what's changed).
> >
> > Prior to commit bbd8ecc05434 ("net/enic: remove PMD log type
> references"):
> >
> > enic prints vNIC config related messages (rq/cq/wq info and such) via
> > dev_info(). And, dev_info() uses LOGTYPE_PMD and the INFO level.
> > LOGTYPE_PMD defaults to the INFO level, so these messages appear by
> > default. Customers and tech support use them for debugging and so on.
> >
> > After the commit:
> >
> > dev_info() is now enic_pmd_logtype, which defaults to NOTICE. The
> > macro is still using the INFO level. So, config messages are
> > suppressed by default. This was never the intention. The current patch
> > tries to fix that by elevating dev_info to dev_notice, because we do
> > want these messages to appear by default. Should have done it as part
> > of the previous commit, but we missed it.
> >
> > Down the line, we will have to guide our customers to exploit dynamic
> > log levels, but not this way (i.e. suddenly hiding messages that they
> > used to see/rely on).
> >
> > Thanks a lot.
> > -Hyong
> >
> 
> Drivers should be silent unless they see a problem.
> We don't want every driver outputting messages by default.
> 
> For your current issue, why not just register the default log level as info?

Yes, that is better and what Ferruh suggested also. It is what the v2 patch is: 
http://patches.dpdk.org/patch/57199/.
Thanks, John

[dpdk-dev] [RFC] Generic flow director/filtering/classification API

[John Daley (johndale)]

Hi, this is an old thread, but I'll reply to this instead of the RFC v2 since 
there is more context here.
Thanks for pushing the new api forward Adrien.
-john daley

> > >>> - Match range of Physical Functions (PFs) on the NIC in a single rule
> > >>>   via masks. For ex: match all traffic coming on several PFs.
> > >>
> > >> The PF and VF pattern items assume there is a single PF associated
> > >> with a DPDK port. VFs are identified with an ID. I basically took
> > >> the same definitions as the existing filter types, perhaps this is
> > >> not enough for Chelsio adapters.
> > >>
> > >> Do you expose more than one PF for a DPDK port?

The Cisco VIC can support multiple PFs per Ethernet port.  These are called 
virtual-nics (VNICs). It would be nice to be able to redirect matched Rx 
packets to another queue on another VNIC.

> > >>
> > >> Anyway, I'd suggest the same approach as above, automatic
> > >> aggregation of rules for performance reasons, otherwise new or
> > >> updated PF/VF pattern items, in which case it would be great if you
> > >> could provide ideal structure definitions for this use case.
> > >>
> > >
> > > In Chelsio hardware, all the ports of a device are exposed via
> > > single PF4. There could be many VFs attached to a PF.  Physical NIC
> > > functions are operational on PF4, while VFs can be attached to PFs 0-3.
> > > So, Chelsio hardware doesn't remain tied on a PF-to-Port, one-to-one
> > > mapping assumption.
> > >
> > > There already seems to be a PF meta-item, but it doesn't seem to
> > > accept any "spec" and "mask" field.  Similarly, the VF meta-item
> > > doesn't seem to accept a "mask" field.  We could probably enable
> > > these fields in the PF and VF meta-items to allow configuration.
> >
I would like to see an ID property added to the PF action meta-item, where 
perhaps a BDF can be specified. This would potentially allow matched Rx packets 
to be redirected to another VNIC and could be paired with the QUEUE action 
meta-item to redirect to a specific queue on a VNIC. The PF ID property set to 
0 would have the current specified behavior or redirecting to the current PF. 
Is something like this possible?

[dpdk-dev] [PATCH] Enic PMD Rx performance improvements

[John Daley (johndale)]

I found a data corruption in further testing, so please reject the patch and 
I'll send a V2.
Thanks,
John

> -Original Message-
> From: John Daley (johndale)
> Sent: Monday, February 15, 2016 9:37 PM
> To: dev at dpdk.org
> Cc: John Daley (johndale) 
> Subject: [PATCH] Enic PMD Rx performance improvements
> 
> This is a rewrite of the receive path of the Enic PMD to simplify the code and
> improve packet rate. Sorry I couldn't figure a way to organize it as a series 
> of
> patches, so I'm submitting it as a single patch.
> thanks,
> john
> 
> johndale (1):
>   ENIC PMD receive path performance improvements.
> 
>  drivers/net/enic/Makefile   |   1 +
>  drivers/net/enic/base/vnic_rq.c |  99 ++
> drivers/net/enic/base/vnic_rq.h | 147 +-
>  drivers/net/enic/enic.h |  16 +-
>  drivers/net/enic/enic_ethdev.c  |  21 +-
>  drivers/net/enic/enic_main.c| 319 +--
>  drivers/net/enic/enic_res.h |  16 +-
>  drivers/net/enic/enic_rx.c  | 413
> 
>  8 files changed, 555 insertions(+), 477 deletions(-)  create mode 100644
> drivers/net/enic/enic_rx.c
> 
> --
> 2.7.0

[dpdk-dev] [RFC] mbuf: remove unused rx error flags

[John Daley (johndale)]

Hi,

> -Original Message-
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, May 10, 2016 1:40 AM
> To: dev at dpdk.org
> Cc: konstantin.ananyev at intel.com; John Daley (johndale)
> ; helin.zhang at intel.com; arnon at qwilt.com;
> rolette at infinite.io
> Subject: [RFC] mbuf: remove unused rx error flags
> 
> Following the discussions from:
> http://dpdk.org/ml/archives/dev/2015-July/021721.html
> http://dpdk.org/ml/archives/dev/2016-April/038143.html
> 
> The value of these flags is 0, making them useless. Today, no example
> application checks them on RX, and only few drivers sets them, and silently
> give wrong packets to the application, which should not happen.
> 
> This patch removes the unused flags from rte_mbuf, and their use in the
> drivers. The enic driver is modified to drop bad packets, but i40e and fm10k
> are kept as they are today and should be fixed to drop bad packets.

Perhaps the change to enic to drop bad packets should be handled as a separate 
patch since it's not strictly related to not removing the use of the flags?

> 
> Fixes: c22265f6 ("mbuf: add new packet flags for i40e")
> Signed-off-by: Olivier Matz 
> ---
> 
> Hi,
> 
> Here is a draft patch that removes the rx mbuf flags, as discussed.
> 
> John, I did not test the patch on the enic driver, so please review it 
> carefully.
> 

The patch works for enic, thanks. There are a few minor changes for performance:
 - put an unlikely in the if (packet_error) conditional
- remove the if (!packet_error) conditional since it will always be true.
Let me know if you would prefer I submit the enic patch separately.

> Comments are welcome.
> 
> Olivier
> 
> 
>  drivers/net/enic/enic_rx.c | 31 ++-
>  drivers/net/fm10k/fm10k_rxtx.c | 16 
>  drivers/net/fm10k/fm10k_rxtx_vec.c |  2 +-
>  drivers/net/i40e/i40e_rxtx.c   | 15 ---
>  lib/librte_mbuf/rte_mbuf.c |  4 
>  lib/librte_mbuf/rte_mbuf.h |  5 +
>  6 files changed, 16 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/net/enic/enic_rx.c b/drivers/net/enic/enic_rx.c index
> b3ad9ea..bad802e 100644
> --- a/drivers/net/enic/enic_rx.c
> +++ b/drivers/net/enic/enic_rx.c
> @@ -144,20 +144,15 @@ enic_cq_rx_desc_n_bytes(struct cq_desc *cqd)  }
> 
>  static inline uint8_t
> -enic_cq_rx_to_pkt_err_flags(struct cq_desc *cqd, uint64_t
> *pkt_err_flags_out)
> +enic_cq_rx_check_err(struct cq_desc *cqd)
>  {
>   struct cq_enet_rq_desc *cqrd = (struct cq_enet_rq_desc *)cqd;
>   uint16_t bwflags;
> - int ret = 0;
> - uint64_t pkt_err_flags = 0;
> 
>   bwflags = enic_cq_rx_desc_bwflags(cqrd);
> - if (unlikely(enic_cq_rx_desc_packet_error(bwflags))) {
> - pkt_err_flags = PKT_RX_MAC_ERR;
> - ret = 1;
> - }
> - *pkt_err_flags_out = pkt_err_flags;
> - return ret;
> + if (unlikely(enic_cq_rx_desc_packet_error(bwflags)))
> + return 1;
> + return 0;
>  }
> 
>  /*
> @@ -253,7 +248,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   struct enic *enic = vnic_dev_priv(rq->vdev);
>   unsigned int rx_id;
>   struct rte_mbuf *nmb, *rxmb;
> - uint16_t nb_rx = 0;
> + uint16_t nb_rx = 0, nb_err = 0;
>   uint16_t nb_hold;
>   struct vnic_cq *cq;
>   volatile struct cq_desc *cqd_ptr;
> @@ -269,7 +264,6 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   volatile struct rq_enet_desc *rqd_ptr;
>   dma_addr_t dma_addr;
>   struct cq_desc cqd;
> - uint64_t ol_err_flags;
>   uint8_t packet_error;
> 
>   /* Check for pkts available */
> @@ -293,7 +287,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   }
> 
>   /* A packet error means descriptor and data are untrusted */
> - packet_error = enic_cq_rx_to_pkt_err_flags(&cqd,
> &ol_err_flags);
> + packet_error = enic_cq_rx_check_err(&cqd);
> 
>   /* Get the mbuf to return and replace with one just allocated
> */
>   rxmb = rq->mbuf_ring[rx_id];
> @@ -318,6 +312,13 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf
> **rx_pkts,
>   rqd_ptr->address = rte_cpu_to_le_64(dma_addr);
>   rqd_ptr->length_type = cpu_to_le16(nmb->buf_len);
> 
> + /* Drop incoming bad packet */
> + if (packet_error) {
> + rte_pktmbuf_free(rxmb);
> + nb_err++;
> + continue;
> +   

[dpdk-dev] [RFC] mbuf: new flag when vlan is stripped

[John Daley (johndale)]

Hi Olivier,

> ...
> This is a draft patch that implements what was previously discussed, except
> the packet_type, which does not exist for vlan today (and I think it is not
> mandatory for now, let's do it in another patch).
> 
> After doing this patch, it appeared that ixgbe was the only driver that had a
> different behavior for the PKT_RX_VLAN_PKT flag. An alternative to this
> patch would be to only change the behavior of the ixgbe driver, and just
> document better document the PKT_RX_VLAN_PKT flags in rte_mbuf.h
> without adding new flags. I think this is a better option.
> 
> Comments are welcome.
>
There are applications depending on the current behavior of PKT_RX_VLAN_PKT as 
confusing as it may be.  I know of one that has VLAN stripping disabled and 
uses the flag to determine if the packet delivered to the app has a VLAN tag. 
This is actually how the flag behaves for ixgbe, and they patched enic and 
maybe other drivers to act accordingly. To avoid breaking the app (and any 
others like it), I think we should keep the flag behavior the same and add the 
new flag PKT_RX_VLAN_STRIPPED .

We should follow on with the new packet type since it enables a nice 
performance improvement by not forcing apps to break open the packet just to 
see if there is a VLAN tag.

Thanks,
john

[dpdk-dev] [vpp-dev] VLAN packets dropped... ?

[John Daley (johndale)]

John,
As just discussed, what you suggest was considered but there are other apps 
depending a different behavior of the flag, so we thought the best thing to do 
is deprecate it. That is part of what Olivier's patch discussed in 
http://www.dpdk.org/ml/archives/dev/2016-May/038786.html does.  Adding a new 
ptype for VLAN tagged packets after the patch is accepted would allow VPP to 
check if the ptype is supported by the PMD and if so, use it to determine if 
the delivered packet actually has a VLAN tag in it. No need to know if 
stripping is enabled/disabled. If the ptype is not supported,  the app would 
have check the packet in SW.

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Lo (loj)
> Sent: Thursday, May 26, 2016 11:11 AM
> To: Ananyev, Konstantin ; Wiles, Keith
> ; Chandrasekar Kannan 
> Cc: vpp-dev ; Zhang, Helin ;
> dev at dpdk.org
> Subject: Re: [dpdk-dev] [vpp-dev] VLAN packets dropped... ?
> 
> Hi Konstantin,
> 
> Thanks for the link to DPDK discussion wrt this VLAN offload flag
> PKT_RX_VLAN_PKT.  It seems the proposal was to deprecate
> PKT_RX_VLAN_PKT  and introduce two new flags PKT_RX_VLAN_STRIPPED
> and PKT_RX_QINQ_STRIPPED.
> 
> It would be a really good idea to keep PKT_RX_VLAN_PKT to indicate at least
> one VLAN tag is present on the packet.  For the case of i40e where its HW
> cannot detect VLAN tag if strip is not enabled, it should be reasonable for 
> the
> i40e DPDK driver software to make a check and set this flag. I would think the
> overhead to check the 1st ethertype field in the MAC header against dot1q
> or dot1ad values in software be pretty minimal.
> 
> Regards,
> John
> 
> 
> -Original Message-
> From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> Sent: Wednesday, May 25, 2016 3:23 PM
> To: John Lo (loj); Wiles, Keith; Chandrasekar Kannan
> Cc: vpp-dev; Zhang, Helin; dev at dpdk.org
> Subject: RE: [vpp-dev] VLAN packets dropped... ?
> 
> 
> > I suppose this has to do with what is expected usage of the
> > PKT_RX_VLAN_PKT offload flag. Is it set only for VLAN packets with the
> > VLAN stripped or should always be set if VLAN is/was present in the
> > received packet. It seems that different DPDK drivers are behaving
> differently which will make it really hard for VPP to take advantage of NIC
> and driver offload capability to provide better performance.
> 
> Yes, that's true ixgbe/igb from one side and i40e do raise PKT_RX_VLAN_PKT
> in a different manner.
> There is an attempt to make it unified across all supported devices:
>  http://dpdk.org/dev/patchwork/patch/12938/
> 
> Though, I am not sure it will help you with your issue.
> As I understand, for you the desired behaviour is:
> If it is a vlan packet, keep the packet intact (don't strip the vlan) and 
> raise
> PKT_RX_VLAN_PK inside mbuf->ol_flags.
> That's what ixgbe is doing with rte_eth_conf.rxmode.hw_vlan_strip==0.
> Correct?
> As far as I know, i40e HW doesn't provide such ability.
> i40e Receive HW Descriptor can only flag was VLAN tag stripped from the
> packet or not, but if stripping is disabled it wouldn't indicate in any way is
> VLAN tag is present inside the packet or not.
> I am CC-ing it to dpdk.org in case I am missing something here.
> Probably someone knows a way to make it work in that way.
> Konstantin
> 
> >
> > If VPP cannot rely on offload flags for VLAN so packets always have to go
> through ethernet-input node, there is a performance cost.
> > For the 10GE case, before the inverse patch of the ixgbe driver, 10GE
> > Rx-vector path removed support of offload flag with DPDK 16.04 and so
> > ethernet-input node is always used. The 10GE IPv4 throughput rate
> > dropped from 6.17MPPSx2 to 4.92MPPSx2 for bidirectional traffic (2
> > streams each with a single IP address as destination) on a single core
> > on my server. Konstantin suggested at that time to use scalar mode which
> does support offload flags properly. The scalar mode did by-pass ethernet-
> input and provided throughput of 5.72MPPS. We ended up inverse patched
> the ixgbe driver to restore vector mode offload flag support as the original
> restriction (the reason offload flag support was removed) would not affect
> VPP.
> >
> > I think for 40GE driver to provide offload flag support in vector mode
> > but not give indication of presence of VLAN tag is just wrong. This make the
> offload flag support useless for VPP.
> >
> > Regards,
> > John
> >
> > -Original Message-
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev at intel.com]
> > Sent: Wednesday, May 25, 2016 11:30 AM
> > To: John Lo (loj); Wiles, Keith; Chandrasekar Kannan
> > Cc: vpp-dev; Zhang, Helin
> > Subject: RE: [vpp-dev] VLAN packets dropped... ?
> >
> >
> > >
> > > I see what you are getting at, Konstantin. The VPP init code does
> > > not enable VLAN strip for Intel NICs as VLAN tag must be in the
> > > packet for sub-interface lookup by ethernet-input node. I agree if
> > > we 

[dpdk-dev] [PATCH v12 0/6] add Tx preparation

[John Daley (johndale)]

Hi,
-john

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, November 28, 2016 3:03 AM
> To: dev at dpdk.org; Rahul Lakkireddy ;
> Stephen Hurd ; Jan Medala
> ; Jakub Palider ; John Daley
> (johndale) ; Adrien Mazarguil
> ; Alejandro Lucero
> ; Harish Patil
> ; Rasesh Mody ; Jerin
> Jacob ; Yuanhan Liu
> ; Yong Wang 
> Cc: Tomasz Kulasek ;
> konstantin.ananyev at intel.com; olivier.matz at 6wind.com
> Subject: Re: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
> 
> We need attention of every PMD developers on this thread.
> 
> Reminder of what Konstantin suggested:
> "
> - if the PMD supports TX offloads AND
> - if to be able use any of these offloads the upper layer SW would have to:
> * modify the contents of the packet OR
> * obey HW specific restrictions
> then it is a PMD developer responsibility to provide tx_prep() that would
> implement expected modifications of the packet contents and restriction
> checks.
> Otherwise, tx_prep() implementation is not required and can be safely set to
> NULL.
> "
> 
> I copy/paste also my previous conclusion:
> 
> Before txprep, there is only one API: the application must prepare the
> packets checksum itself (get_psd_sum in testpmd).
> With txprep, the application have 2 choices: keep doing the job itself or call
> txprep which calls a PMD-specific function.
> The question is: does non-Intel drivers need a checksum preparation for
> TSO?
> Will it behave well if txprep does nothing in these drivers?
> 
> When looking at the code, most of drivers handle the TSO flags.
> But it is hard to know whether they rely on the pseudo checksum or not.
> 
> git grep -l 'PKT_TX_UDP_CKSUM\|PKT_TX_TCP_CKSUM\|PKT_TX_TCP_SEG'
> drivers/net/
> 
> drivers/net/bnxt/bnxt_txr.c
> drivers/net/cxgbe/sge.c
> drivers/net/e1000/em_rxtx.c
> drivers/net/e1000/igb_rxtx.c
> drivers/net/ena/ena_ethdev.c
> drivers/net/enic/enic_rxtx.c
> drivers/net/fm10k/fm10k_rxtx.c
> drivers/net/i40e/i40e_rxtx.c
> drivers/net/ixgbe/ixgbe_rxtx.c
> drivers/net/mlx4/mlx4.c
> drivers/net/mlx5/mlx5_rxtx.c
> drivers/net/nfp/nfp_net.c
> drivers/net/qede/qede_rxtx.c
> drivers/net/thunderx/nicvf_rxtx.c
> drivers/net/virtio/virtio_rxtx.c
> drivers/net/vmxnet3/vmxnet3_rxtx.c
> 
> Please, we need a comment for each driver saying "it is OK, we do not need
> any checksum preparation for TSO"
> or
> "yes we have to implement tx_prepare or TSO will not work in this mode"

I like the idea of tx prep since it should make for cleaner apps.

For enic, I believe the answer is " it is OK, we do not need any checksum 
preparation".

Prior to now, it was necessary to set IP checksum to 0 and put in a TCP/UDP 
pseudo header. But there is a hardware overwrite of checksums option which 
makes preparation in software unnecessary and it is testing out well so far. I 
plan to enable it in 17.02. TSO is also being enabled for 17.02 and it does not 
look like any prep is required. So I'm going with " txprep NULL pointer is OK 
for enic", but may have to change my mind if something comes up in testing.

-john

[dpdk-dev] [PATCH 4/4] net/enic: extend fdir support for 1300 series adapters

[John Daley (johndale)]

> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yigit at intel.com]
> Sent: Tuesday, October 11, 2016 2:22 AM
> To: John Daley (johndale) ;
> bruce.richardson at intel.com
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 4/4] net/enic: extend fdir support for 1300
> series adapters
> 
> Hi John,
> 
> On 9/29/2016 9:56 PM, John Daley wrote:
> > 1300 series Cisco adapter firmware version 2.0(13) for UCS C-series
> > servers and 3.1(2) for blade servers supports more filtering
> > capabilities. The feature can be enabled via Cisco CIMC or USCM with
> > the 'advanced filters' radio button. When enabled, the these
> > additional flow director modes are available:
> > RTE_ETH_FLOW_NONFRAG_IPV4_OTHER
> > RTE_ETH_FLOW_NONFRAG_IPV4_SCTP
> > RTE_ETH_FLOW_NONFRAG_IPV6_UDP
> > RTE_ETH_FLOW_NONFRAG_IPV6_TCP
> > RTE_ETH_FLOW_NONFRAG_IPV6_SCTP
> > RTE_ETH_FLOW_NONFRAG_IPV6_OTHER
> >
> > Changes:
> > - Detect and set an 'advanced filters' flag dependent on the adapter
> >   capability.
> > - Implement RTE_ETH_FILTER_INFO filter op to return the flow types
> >   available dependent on whether advanced filters are enabled.
> > - Use a function pointer to select how filters are added to the adapter:
> >   copy_fltr_v1() for older firmware/adapters or copy_fltr_v2() for
> >   adapters which support advanced filters.
> > - Apply fdir global masks to filters when in advanced filter mode.
> > - Update documentation.
> >
> > Signed-off-by: John Daley 
> > Reviewed-by: Nelson Escobar 
> > ---
> 
> <...>
> 
> >
> > +void enic_fdir_info_get(struct enic *enic, struct rte_eth_fdir_info
> > +*info) {
> > +   info->mode = enic->fdir.modes;
> 
> This cause a icc build error:
> .../drivers/net/enic/enic_clsf.c(77):
> error #188: enumerated type mixed with another type
> info->mode = enic->fdir.modes;
>^
> 
> Just casting to the enum fixes it:
> +   info->mode = (enum rte_fdir_mode)enic->fdir.modes;
> 
> Since the modification is trivial, if you agree with the change, we can apply 
> it
> without needing a new version of the patch?
> 

Looks good, thank you and sorry for the trouble.
-john

> <...>