[PATCH] net/enic: add private API to set ingress VLAN rewrite mode

[PATCH] net/enic: add private API to set ingress VLAN rewrite mode

[Hyong Youb Kim]

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.

Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
---
 drivers/net/enic/Makefile                 |  3 +++
 drivers/net/enic/enic.h                   |  1 +
 drivers/net/enic/enic_ethdev.c            |  7 +++++++
 drivers/net/enic/meson.build              |  1 +
 drivers/net/enic/rte_pmd_enic.c           | 33 +++++++++++++++++++++++++++++++
 drivers/net/enic/rte_pmd_enic.h           | 27 +++++++++++++++++++++++++
 drivers/net/enic/rte_pmd_enic_version.map |  6 ++++++
 7 files changed, 78 insertions(+)
 create mode 100644 drivers/net/enic/rte_pmd_enic.c
 create mode 100644 drivers/net/enic/rte_pmd_enic.h

diff --git a/drivers/net/enic/Makefile b/drivers/net/enic/Makefile
index 04bae35e3..60373fa30 100644
--- a/drivers/net/enic/Makefile
+++ b/drivers/net/enic/Makefile
@@ -37,6 +37,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_wq.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_dev.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_intr.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_rq.c
+SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += rte_pmd_enic.c
 
 # The current implementation assumes 64-bit pointers
 CC_AVX2_SUPPORT=0
@@ -66,4 +67,6 @@ ifeq ($(CC_AVX2_SUPPORT), 1)
 	SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_rxtx_vec_avx2.c
 endif
 
+SYMLINK-$(CONFIG_RTE_LIBRTE_ENIC_PMD)-include := rte_pmd_enic.h
+
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index fa4d5590e..94563fe70 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -279,6 +279,7 @@ enic_ring_incr(uint32_t n_descriptors, uint32_t idx)
 	return idx;
 }
 
+bool enic_dev_is_enic(struct rte_eth_dev *dev);
 void enic_fdir_stats_get(struct enic *enic,
 			 struct rte_eth_fdir_stats *stats);
 int enic_fdir_add_fltr(struct enic *enic,
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 8d14d8ac7..3a6aba3a2 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1199,6 +1199,13 @@ static struct rte_pci_driver rte_enic_pmd = {
 	.remove = eth_enic_pci_remove,
 };
 
+bool
+enic_dev_is_enic(struct rte_eth_dev *dev)
+{
+	return strcmp(dev->device->driver->name,
+		      rte_enic_pmd.driver.name) == 0;
+}
+
 RTE_PMD_REGISTER_PCI(net_enic, rte_enic_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_enic, pci_id_enic_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_enic, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/enic/meson.build b/drivers/net/enic/meson.build
index c381f1496..c5520c17b 100644
--- a/drivers/net/enic/meson.build
+++ b/drivers/net/enic/meson.build
@@ -13,6 +13,7 @@ sources = files(
 	'enic_main.c',
 	'enic_res.c',
 	'enic_rxtx.c',
+	'rte_pmd_enic.c',
 	)
 deps += ['hash']
 includes += include_directories('base')
diff --git a/drivers/net/enic/rte_pmd_enic.c b/drivers/net/enic/rte_pmd_enic.c
new file mode 100644
index 000000000..7c98edd35
--- /dev/null
+++ b/drivers/net/enic/rte_pmd_enic.c
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2019 Cisco Systems, Inc.  All rights reserved.
+ */
+
+#include <rte_ethdev_driver.h>
+
+#include "enic_compat.h"
+#include "enic.h"
+#include "rte_pmd_enic.h"
+
+int
+rte_pmd_enic_set_ig_vlan_rewrite(uint16_t port, uint8_t mode)
+{
+	struct rte_eth_dev *dev;
+	struct enic *enic;
+	int err;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+	dev = &rte_eth_devices[port];
+	if (!enic_dev_is_enic(dev))
+		return -ENOTSUP;
+	if (mode > RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_PASS_THRU)
+		return -EINVAL;
+	enic = pmd_priv(dev);
+	dev_debug(enic, "Set ig_vlan_rewrite_mode=%u\n", mode);
+	err = vnic_dev_set_ig_vlan_rewrite_mode(enic->vdev, mode);
+	if (err) {
+		dev_err(enic, "Failed to set ingress vlan rewrite mode\n");
+		return err;
+	}
+	enic->ig_vlan_rewrite_mode = mode;
+	return 0;
+}
diff --git a/drivers/net/enic/rte_pmd_enic.h b/drivers/net/enic/rte_pmd_enic.h
new file mode 100644
index 000000000..d4486929a
--- /dev/null
+++ b/drivers/net/enic/rte_pmd_enic.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2019 Cisco Systems, Inc.  All rights reserved.
+ */
+
+#ifndef _RTE_PMD_ENIC_H_
+#define _RTE_PMD_ENIC_H_
+
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_DEFAULT_TRUNK              0
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_UNTAG_DEFAULT_VLAN         1
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_PRIORITY_TAG_DEFAULT_VLAN  2
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_PASS_THRU                  3
+
+/**
+ * Set the ingress VLAN rewrite mode.
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param mode
+ *    Rewrite mode.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int rte_pmd_enic_set_ig_vlan_rewrite(uint16_t port, uint8_t mode);
+
+#endif /* _RTE_PMD_ENIC_H_ */
diff --git a/drivers/net/enic/rte_pmd_enic_version.map b/drivers/net/enic/rte_pmd_enic_version.map
index ef3539840..0abef048d 100644
--- a/drivers/net/enic/rte_pmd_enic_version.map
+++ b/drivers/net/enic/rte_pmd_enic_version.map
@@ -2,3 +2,9 @@ DPDK_2.0 {
 
 	local: *;
 };
+
+DPDK_19.05 {
+	global:
+
+	rte_pmd_enic_set_ig_vlan_rewrite;
+} DPDK_2.0;
-- 
2.16.2

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

[Hyong Youb Kim]

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.

Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
---
v2:
* Move symbol to the experimental section

 drivers/net/enic/Makefile                 |  3 +++
 drivers/net/enic/enic.h                   |  1 +
 drivers/net/enic/enic_ethdev.c            |  7 +++++++
 drivers/net/enic/meson.build              |  1 +
 drivers/net/enic/rte_pmd_enic.c           | 33 +++++++++++++++++++++++++++++++
 drivers/net/enic/rte_pmd_enic.h           | 28 ++++++++++++++++++++++++++
 drivers/net/enic/rte_pmd_enic_version.map |  6 ++++++
 7 files changed, 79 insertions(+)
 create mode 100644 drivers/net/enic/rte_pmd_enic.c
 create mode 100644 drivers/net/enic/rte_pmd_enic.h

diff --git a/drivers/net/enic/Makefile b/drivers/net/enic/Makefile
index 04bae35e3..60373fa30 100644
--- a/drivers/net/enic/Makefile
+++ b/drivers/net/enic/Makefile
@@ -37,6 +37,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_wq.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_dev.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_intr.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += base/vnic_rq.c
+SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += rte_pmd_enic.c
 
 # The current implementation assumes 64-bit pointers
 CC_AVX2_SUPPORT=0
@@ -66,4 +67,6 @@ ifeq ($(CC_AVX2_SUPPORT), 1)
 	SRCS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += enic_rxtx_vec_avx2.c
 endif
 
+SYMLINK-$(CONFIG_RTE_LIBRTE_ENIC_PMD)-include := rte_pmd_enic.h
+
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index fa4d5590e..94563fe70 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -279,6 +279,7 @@ enic_ring_incr(uint32_t n_descriptors, uint32_t idx)
 	return idx;
 }
 
+bool enic_dev_is_enic(struct rte_eth_dev *dev);
 void enic_fdir_stats_get(struct enic *enic,
 			 struct rte_eth_fdir_stats *stats);
 int enic_fdir_add_fltr(struct enic *enic,
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 8d14d8ac7..3a6aba3a2 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -1199,6 +1199,13 @@ static struct rte_pci_driver rte_enic_pmd = {
 	.remove = eth_enic_pci_remove,
 };
 
+bool
+enic_dev_is_enic(struct rte_eth_dev *dev)
+{
+	return strcmp(dev->device->driver->name,
+		      rte_enic_pmd.driver.name) == 0;
+}
+
 RTE_PMD_REGISTER_PCI(net_enic, rte_enic_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_enic, pci_id_enic_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_enic, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/enic/meson.build b/drivers/net/enic/meson.build
index c381f1496..c5520c17b 100644
--- a/drivers/net/enic/meson.build
+++ b/drivers/net/enic/meson.build
@@ -13,6 +13,7 @@ sources = files(
 	'enic_main.c',
 	'enic_res.c',
 	'enic_rxtx.c',
+	'rte_pmd_enic.c',
 	)
 deps += ['hash']
 includes += include_directories('base')
diff --git a/drivers/net/enic/rte_pmd_enic.c b/drivers/net/enic/rte_pmd_enic.c
new file mode 100644
index 000000000..97804baa6
--- /dev/null
+++ b/drivers/net/enic/rte_pmd_enic.c
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2019 Cisco Systems, Inc.  All rights reserved.
+ */
+
+#include <rte_ethdev_driver.h>
+
+#include "enic_compat.h"
+#include "enic.h"
+#include "rte_pmd_enic.h"
+
+int __rte_experimental
+rte_pmd_enic_set_ig_vlan_rewrite(uint16_t port, uint8_t mode)
+{
+	struct rte_eth_dev *dev;
+	struct enic *enic;
+	int err;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
+	dev = &rte_eth_devices[port];
+	if (!enic_dev_is_enic(dev))
+		return -ENOTSUP;
+	if (mode > RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_PASS_THRU)
+		return -EINVAL;
+	enic = pmd_priv(dev);
+	dev_debug(enic, "Set ig_vlan_rewrite_mode=%u\n", mode);
+	err = vnic_dev_set_ig_vlan_rewrite_mode(enic->vdev, mode);
+	if (err) {
+		dev_err(enic, "Failed to set ingress vlan rewrite mode\n");
+		return err;
+	}
+	enic->ig_vlan_rewrite_mode = mode;
+	return 0;
+}
diff --git a/drivers/net/enic/rte_pmd_enic.h b/drivers/net/enic/rte_pmd_enic.h
new file mode 100644
index 000000000..92c3bd6a9
--- /dev/null
+++ b/drivers/net/enic/rte_pmd_enic.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2019 Cisco Systems, Inc.  All rights reserved.
+ */
+
+#ifndef _RTE_PMD_ENIC_H_
+#define _RTE_PMD_ENIC_H_
+
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_DEFAULT_TRUNK              0
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_UNTAG_DEFAULT_VLAN         1
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_PRIORITY_TAG_DEFAULT_VLAN  2
+#define RTE_PMD_ENIC_IG_VLAN_REWRITE_MODE_PASS_THRU                  3
+
+/**
+ * Set the ingress VLAN rewrite mode.
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param mode
+ *    Rewrite mode.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int __rte_experimental
+rte_pmd_enic_set_ig_vlan_rewrite(uint16_t port, uint8_t mode);
+
+#endif /* _RTE_PMD_ENIC_H_ */
diff --git a/drivers/net/enic/rte_pmd_enic_version.map b/drivers/net/enic/rte_pmd_enic_version.map
index ef3539840..847597b9a 100644
--- a/drivers/net/enic/rte_pmd_enic_version.map
+++ b/drivers/net/enic/rte_pmd_enic_version.map
@@ -2,3 +2,9 @@ DPDK_2.0 {
 
 	local: *;
 };
+
+EXPERIMENTAL {
+	global:
+
+	rte_pmd_enic_set_ig_vlan_rewrite;
+};
-- 
2.16.2

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

[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 ...

[1]
https://git.dpdk.org/dpdk/tree/lib/librte_rawdev/rte_rawdev.c?h=v19.02#n186

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

[Thomas Monjalon]

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?

Re: [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 <thomas@monjalon.net>
> Sent: Wednesday, March 13, 2019 1:36 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com>; Hyong Youb Kim (hyonkim)
> <hyonkim@cisco.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>; Qi Zhang
> <qi.z.zhang@intel.com>; dev@dpdk.org; John Daley (johndale)
> <johndale@cisco.com>; Shahaf Shuler <shahafs@mellanox.com>; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; David Marchand
> <david.marchand@6wind.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> 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: [PATCH v2] net/enic: add private API to set ingress VLAN rewrite mode

[Thomas Monjalon]

13/03/2019 22:11, John Daley (johndale):
> From: Thomas Monjalon <thomas@monjalon.net>
> > 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.

It looks like an application will always set this flag or never.
So I don't see the need for an API function.
Why VPP prefers set this flag later?
Would it be better to have some driver-specific flags, no matter the ports?

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

[Hyong Youb Kim]

On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> 13/03/2019 22:11, John Daley (johndale):
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 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.
> 
> It looks like an application will always set this flag or never.
> So I don't see the need for an API function.
> Why VPP prefers set this flag later?
> Would it be better to have some driver-specific flags, no matter the ports?
> 
> 

As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.

1. Leave everything to the user.

Let the human user specify NIC-specific settings (e.g. devarg,
not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
app simply passes these to DPDK and does nothing else. Devargs are
passed to rte_eal_init. Other settings are applied during the
configure phase after rte_eal_init.

For example, OVS seems to go for a smallest common denominator that
works with most NICs out of the box. Otherwiese, it kinda falls into
this camp.

For a problematic NIC that needs user intervention, troubleshooting
goes like this :-)
- Install app.
- Run with settings that worked on a previous machine.
- Some features suddenly do not work.
- Google search this and that ("why this does not work on this server?").
- Contact vendors.
- Find out this NIC has unexpected behavior.
- Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
- Now the features work.

2. Hide ugly tweaks from the user.

VPP falls into this camp. The user specifies BDFs in the config (no
devargs). The app calls rte_eal_init(BDFs), iterates through the
discovered ports, applies whatever NIC-specific settings necessary
during the configure phase (i.e. do this for vendor A NIC, do that for
vendor B NIC), and then start the ports.

The ingress vlan rewrite mode is devarg now, so is not usable in this
model. One way around it is a private API. Driver specific flags
during the configure phase would also work fine. Though, enic might be
the only user of those flags.

Thanks.
-Hyong

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

[Thomas Monjalon]

14/03/2019 03:58, Hyong Youb Kim:
> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> > 13/03/2019 22:11, John Daley (johndale):
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 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.
> > 
> > It looks like an application will always set this flag or never.
> > So I don't see the need for an API function.
> > Why VPP prefers set this flag later?
> > Would it be better to have some driver-specific flags, no matter the ports?
> 
> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
> 
> 1. Leave everything to the user.
> 
> Let the human user specify NIC-specific settings (e.g. devarg,
> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
> app simply passes these to DPDK and does nothing else. Devargs are
> passed to rte_eal_init. Other settings are applied during the
> configure phase after rte_eal_init.
> 
> For example, OVS seems to go for a smallest common denominator that
> works with most NICs out of the box. Otherwiese, it kinda falls into
> this camp.
> 
> For a problematic NIC that needs user intervention, troubleshooting
> goes like this :-)
> - Install app.
> - Run with settings that worked on a previous machine.
> - Some features suddenly do not work.
> - Google search this and that ("why this does not work on this server?").
> - Contact vendors.
> - Find out this NIC has unexpected behavior.
> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
> - Now the features work.
> 
> 2. Hide ugly tweaks from the user.
> 
> VPP falls into this camp. The user specifies BDFs in the config (no
> devargs). The app calls rte_eal_init(BDFs), iterates through the
> discovered ports, applies whatever NIC-specific settings necessary
> during the configure phase (i.e. do this for vendor A NIC, do that for
> vendor B NIC), and then start the ports.
> 
> The ingress vlan rewrite mode is devarg now, so is not usable in this
> model. One way around it is a private API. Driver specific flags
> during the configure phase would also work fine. Though, enic might be
> the only user of those flags.

I think DPDK needs some driver configuration.
Currently the config is done per device with devargs.
The next devargs format allow this:
	driver=enic,rewrite=on
and it can be passed to rte_eal_init().

We did not progress on the implementation of this format in recent months,
but you are welcome to help!
Instead of passing devargs in the whitelist/blacklist options,
we should introduce a new option, like --dev.

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

[Ferruh Yigit]

On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
> 14/03/2019 03:58, Hyong Youb Kim:
>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
>>> 13/03/2019 22:11, John Daley (johndale):
>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>> 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.
>>>
>>> It looks like an application will always set this flag or never.
>>> So I don't see the need for an API function.
>>> Why VPP prefers set this flag later?
>>> Would it be better to have some driver-specific flags, no matter the ports?
>>
>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
>>
>> 1. Leave everything to the user.
>>
>> Let the human user specify NIC-specific settings (e.g. devarg,
>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
>> app simply passes these to DPDK and does nothing else. Devargs are
>> passed to rte_eal_init. Other settings are applied during the
>> configure phase after rte_eal_init.
>>
>> For example, OVS seems to go for a smallest common denominator that
>> works with most NICs out of the box. Otherwiese, it kinda falls into
>> this camp.
>>
>> For a problematic NIC that needs user intervention, troubleshooting
>> goes like this :-)
>> - Install app.
>> - Run with settings that worked on a previous machine.
>> - Some features suddenly do not work.
>> - Google search this and that ("why this does not work on this server?").
>> - Contact vendors.
>> - Find out this NIC has unexpected behavior.
>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
>> - Now the features work.
>>
>> 2. Hide ugly tweaks from the user.
>>
>> VPP falls into this camp. The user specifies BDFs in the config (no
>> devargs). The app calls rte_eal_init(BDFs), iterates through the
>> discovered ports, applies whatever NIC-specific settings necessary
>> during the configure phase (i.e. do this for vendor A NIC, do that for
>> vendor B NIC), and then start the ports.
>>
>> The ingress vlan rewrite mode is devarg now, so is not usable in this
>> model. One way around it is a private API. Driver specific flags
>> during the configure phase would also work fine. Though, enic might be
>> the only user of those flags.
> 
> I think DPDK needs some driver configuration.
> Currently the config is done per device with devargs.
> The next devargs format allow this:
> 	driver=enic,rewrite=on
> and it can be passed to rte_eal_init().
> 
> We did not progress on the implementation of this format in recent months,
> but you are welcome to help!
> Instead of passing devargs in the whitelist/blacklist options,
> we should introduce a new option, like --dev.

But it will be still devarg in new implementation.
I guess for this use case, there is a need to pass this information from an API.
Options can be:
1- PMD specific API
2- Extend ethdev dev_ops for each usecase
3- Have a generic API, as suggested above
4- Extend configure to accept flags

I don't see a winner in above list, each has some problems. Any comment on how
to proceed?

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

[Thomas Monjalon]

19/03/2019 18:29, Ferruh Yigit:
> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
> > 14/03/2019 03:58, Hyong Youb Kim:
> >> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> >>> 13/03/2019 22:11, John Daley (johndale):
> >>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>> 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.
> >>>
> >>> It looks like an application will always set this flag or never.
> >>> So I don't see the need for an API function.
> >>> Why VPP prefers set this flag later?
> >>> Would it be better to have some driver-specific flags, no matter the ports?
> >>
> >> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
> >>
> >> 1. Leave everything to the user.
> >>
> >> Let the human user specify NIC-specific settings (e.g. devarg,
> >> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
> >> app simply passes these to DPDK and does nothing else. Devargs are
> >> passed to rte_eal_init. Other settings are applied during the
> >> configure phase after rte_eal_init.
> >>
> >> For example, OVS seems to go for a smallest common denominator that
> >> works with most NICs out of the box. Otherwiese, it kinda falls into
> >> this camp.
> >>
> >> For a problematic NIC that needs user intervention, troubleshooting
> >> goes like this :-)
> >> - Install app.
> >> - Run with settings that worked on a previous machine.
> >> - Some features suddenly do not work.
> >> - Google search this and that ("why this does not work on this server?").
> >> - Contact vendors.
> >> - Find out this NIC has unexpected behavior.
> >> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
> >> - Now the features work.
> >>
> >> 2. Hide ugly tweaks from the user.
> >>
> >> VPP falls into this camp. The user specifies BDFs in the config (no
> >> devargs). The app calls rte_eal_init(BDFs), iterates through the
> >> discovered ports, applies whatever NIC-specific settings necessary
> >> during the configure phase (i.e. do this for vendor A NIC, do that for
> >> vendor B NIC), and then start the ports.
> >>
> >> The ingress vlan rewrite mode is devarg now, so is not usable in this
> >> model. One way around it is a private API. Driver specific flags
> >> during the configure phase would also work fine. Though, enic might be
> >> the only user of those flags.
> > 
> > I think DPDK needs some driver configuration.
> > Currently the config is done per device with devargs.
> > The next devargs format allow this:
> > 	driver=enic,rewrite=on
> > and it can be passed to rte_eal_init().
> > 
> > We did not progress on the implementation of this format in recent months,
> > but you are welcome to help!
> > Instead of passing devargs in the whitelist/blacklist options,
> > we should introduce a new option, like --dev.
> 
> But it will be still devarg in new implementation.

With the new syntax, no need to specify a device.
We can match a driver or multiple drivers sharing the same property.

> I guess for this use case, there is a need to pass this information from an API.
> Options can be:
> 1- PMD specific API
> 2- Extend ethdev dev_ops for each usecase
> 3- Have a generic API, as suggested above
> 4- Extend configure to accept flags
> 
> I don't see a winner in above list, each has some problems. Any comment on how
> to proceed?

I don't see a problem with the devargs approach.

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

[Ferruh Yigit]

On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
> 19/03/2019 18:29, Ferruh Yigit:
>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
>>> 14/03/2019 03:58, Hyong Youb Kim:
>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
>>>>> 13/03/2019 22:11, John Daley (johndale):
>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>> 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.
>>>>>
>>>>> It looks like an application will always set this flag or never.
>>>>> So I don't see the need for an API function.
>>>>> Why VPP prefers set this flag later?
>>>>> Would it be better to have some driver-specific flags, no matter the ports?
>>>>
>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
>>>>
>>>> 1. Leave everything to the user.
>>>>
>>>> Let the human user specify NIC-specific settings (e.g. devarg,
>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
>>>> app simply passes these to DPDK and does nothing else. Devargs are
>>>> passed to rte_eal_init. Other settings are applied during the
>>>> configure phase after rte_eal_init.
>>>>
>>>> For example, OVS seems to go for a smallest common denominator that
>>>> works with most NICs out of the box. Otherwiese, it kinda falls into
>>>> this camp.
>>>>
>>>> For a problematic NIC that needs user intervention, troubleshooting
>>>> goes like this :-)
>>>> - Install app.
>>>> - Run with settings that worked on a previous machine.
>>>> - Some features suddenly do not work.
>>>> - Google search this and that ("why this does not work on this server?").
>>>> - Contact vendors.
>>>> - Find out this NIC has unexpected behavior.
>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
>>>> - Now the features work.
>>>>
>>>> 2. Hide ugly tweaks from the user.
>>>>
>>>> VPP falls into this camp. The user specifies BDFs in the config (no
>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
>>>> discovered ports, applies whatever NIC-specific settings necessary
>>>> during the configure phase (i.e. do this for vendor A NIC, do that for
>>>> vendor B NIC), and then start the ports.
>>>>
>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
>>>> model. One way around it is a private API. Driver specific flags
>>>> during the configure phase would also work fine. Though, enic might be
>>>> the only user of those flags.
>>>
>>> I think DPDK needs some driver configuration.
>>> Currently the config is done per device with devargs.
>>> The next devargs format allow this:
>>> 	driver=enic,rewrite=on
>>> and it can be passed to rte_eal_init().
>>>
>>> We did not progress on the implementation of this format in recent months,
>>> but you are welcome to help!
>>> Instead of passing devargs in the whitelist/blacklist options,
>>> we should introduce a new option, like --dev.
>>
>> But it will be still devarg in new implementation.
> 
> With the new syntax, no need to specify a device.
> We can match a driver or multiple drivers sharing the same property.
> 
>> I guess for this use case, there is a need to pass this information from an API.
>> Options can be:
>> 1- PMD specific API
>> 2- Extend ethdev dev_ops for each usecase
>> 3- Have a generic API, as suggested above
>> 4- Extend configure to accept flags
>>
>> I don't see a winner in above list, each has some problems. Any comment on how
>> to proceed?
> 
> I don't see a problem with the devargs approach.

Devargs either passed via command line to DPDK application, or parameter to
hotplug APIs.
If someone wants to use regular probe without any command line argument, and
later configure the device via an API, can devargs be used?

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

[Thomas Monjalon]

19/03/2019 19:00, Ferruh Yigit:
> On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
> > 19/03/2019 18:29, Ferruh Yigit:
> >> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
> >>> 14/03/2019 03:58, Hyong Youb Kim:
> >>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> >>>>> 13/03/2019 22:11, John Daley (johndale):
> >>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>> 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.
> >>>>>
> >>>>> It looks like an application will always set this flag or never.
> >>>>> So I don't see the need for an API function.
> >>>>> Why VPP prefers set this flag later?
> >>>>> Would it be better to have some driver-specific flags, no matter the ports?
> >>>>
> >>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
> >>>>
> >>>> 1. Leave everything to the user.
> >>>>
> >>>> Let the human user specify NIC-specific settings (e.g. devarg,
> >>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
> >>>> app simply passes these to DPDK and does nothing else. Devargs are
> >>>> passed to rte_eal_init. Other settings are applied during the
> >>>> configure phase after rte_eal_init.
> >>>>
> >>>> For example, OVS seems to go for a smallest common denominator that
> >>>> works with most NICs out of the box. Otherwiese, it kinda falls into
> >>>> this camp.
> >>>>
> >>>> For a problematic NIC that needs user intervention, troubleshooting
> >>>> goes like this :-)
> >>>> - Install app.
> >>>> - Run with settings that worked on a previous machine.
> >>>> - Some features suddenly do not work.
> >>>> - Google search this and that ("why this does not work on this server?").
> >>>> - Contact vendors.
> >>>> - Find out this NIC has unexpected behavior.
> >>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
> >>>> - Now the features work.
> >>>>
> >>>> 2. Hide ugly tweaks from the user.
> >>>>
> >>>> VPP falls into this camp. The user specifies BDFs in the config (no
> >>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
> >>>> discovered ports, applies whatever NIC-specific settings necessary
> >>>> during the configure phase (i.e. do this for vendor A NIC, do that for
> >>>> vendor B NIC), and then start the ports.
> >>>>
> >>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
> >>>> model. One way around it is a private API. Driver specific flags
> >>>> during the configure phase would also work fine. Though, enic might be
> >>>> the only user of those flags.
> >>>
> >>> I think DPDK needs some driver configuration.
> >>> Currently the config is done per device with devargs.
> >>> The next devargs format allow this:
> >>> 	driver=enic,rewrite=on
> >>> and it can be passed to rte_eal_init().
> >>>
> >>> We did not progress on the implementation of this format in recent months,
> >>> but you are welcome to help!
> >>> Instead of passing devargs in the whitelist/blacklist options,
> >>> we should introduce a new option, like --dev.
> >>
> >> But it will be still devarg in new implementation.
> > 
> > With the new syntax, no need to specify a device.
> > We can match a driver or multiple drivers sharing the same property.
> > 
> >> I guess for this use case, there is a need to pass this information from an API.
> >> Options can be:
> >> 1- PMD specific API
> >> 2- Extend ethdev dev_ops for each usecase
> >> 3- Have a generic API, as suggested above
> >> 4- Extend configure to accept flags
> >>
> >> I don't see a winner in above list, each has some problems. Any comment on how
> >> to proceed?
> > 
> > I don't see a problem with the devargs approach.
> 
> Devargs either passed via command line to DPDK application, or parameter to
> hotplug APIs.

The application can pass whatever it wants to EAL.
In the case described above, the application wants to enable
a mode of the driver for all its devices.
That's why I think the right solution is a driver option,
which can be achieved with the new devargs syntax.

> If someone wants to use regular probe without any command line argument, and
> later configure the device via an API, can devargs be used?

This is a scenario different of what is asked above.
In the case of a specific configuration of one device,
we have three choices.
These are your suggestions, with my comments:
	1- PMD specific API
	2- Extend ethdev dev_ops for each usecase
	(3- Have a generic API) = choice 2
	(4- Extend configure to accept flags) = choice 1
This is a choice 3:
	- no support of exotic features

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

[Ferruh Yigit]

On 3/19/2019 8:30 PM, Thomas Monjalon wrote:
> 19/03/2019 19:00, Ferruh Yigit:
>> On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
>>> 19/03/2019 18:29, Ferruh Yigit:
>>>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
>>>>> 14/03/2019 03:58, Hyong Youb Kim:
>>>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
>>>>>>> 13/03/2019 22:11, John Daley (johndale):
>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>> 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.
>>>>>>>
>>>>>>> It looks like an application will always set this flag or never.
>>>>>>> So I don't see the need for an API function.
>>>>>>> Why VPP prefers set this flag later?
>>>>>>> Would it be better to have some driver-specific flags, no matter the ports?
>>>>>>
>>>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
>>>>>>
>>>>>> 1. Leave everything to the user.
>>>>>>
>>>>>> Let the human user specify NIC-specific settings (e.g. devarg,
>>>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
>>>>>> app simply passes these to DPDK and does nothing else. Devargs are
>>>>>> passed to rte_eal_init. Other settings are applied during the
>>>>>> configure phase after rte_eal_init.
>>>>>>
>>>>>> For example, OVS seems to go for a smallest common denominator that
>>>>>> works with most NICs out of the box. Otherwiese, it kinda falls into
>>>>>> this camp.
>>>>>>
>>>>>> For a problematic NIC that needs user intervention, troubleshooting
>>>>>> goes like this :-)
>>>>>> - Install app.
>>>>>> - Run with settings that worked on a previous machine.
>>>>>> - Some features suddenly do not work.
>>>>>> - Google search this and that ("why this does not work on this server?").
>>>>>> - Contact vendors.
>>>>>> - Find out this NIC has unexpected behavior.
>>>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
>>>>>> - Now the features work.
>>>>>>
>>>>>> 2. Hide ugly tweaks from the user.
>>>>>>
>>>>>> VPP falls into this camp. The user specifies BDFs in the config (no
>>>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
>>>>>> discovered ports, applies whatever NIC-specific settings necessary
>>>>>> during the configure phase (i.e. do this for vendor A NIC, do that for
>>>>>> vendor B NIC), and then start the ports.
>>>>>>
>>>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
>>>>>> model. One way around it is a private API. Driver specific flags
>>>>>> during the configure phase would also work fine. Though, enic might be
>>>>>> the only user of those flags.
>>>>>
>>>>> I think DPDK needs some driver configuration.
>>>>> Currently the config is done per device with devargs.
>>>>> The next devargs format allow this:
>>>>> 	driver=enic,rewrite=on
>>>>> and it can be passed to rte_eal_init().
>>>>>
>>>>> We did not progress on the implementation of this format in recent months,
>>>>> but you are welcome to help!
>>>>> Instead of passing devargs in the whitelist/blacklist options,
>>>>> we should introduce a new option, like --dev.
>>>>
>>>> But it will be still devarg in new implementation.
>>>
>>> With the new syntax, no need to specify a device.
>>> We can match a driver or multiple drivers sharing the same property.
>>>
>>>> I guess for this use case, there is a need to pass this information from an API.
>>>> Options can be:
>>>> 1- PMD specific API
>>>> 2- Extend ethdev dev_ops for each usecase
>>>> 3- Have a generic API, as suggested above
>>>> 4- Extend configure to accept flags
>>>>
>>>> I don't see a winner in above list, each has some problems. Any comment on how
>>>> to proceed?
>>>
>>> I don't see a problem with the devargs approach.
>>
>> Devargs either passed via command line to DPDK application, or parameter to
>> hotplug APIs.
> 
> The application can pass whatever it wants to EAL.

This means changing current device probe logic completely, right.
Instead of probing everything on start, probe nothing and application add
devices via eal (hotplug) API calls with providing devargs.
I have no issue with this picture, only it doesn't look soon.

> In the case described above, the application wants to enable
> a mode of the driver for all its devices.
> That's why I think the right solution is a driver option,
> which can be achieved with the new devargs syntax.
> 
>> If someone wants to use regular probe without any command line argument, and
>> later configure the device via an API, can devargs be used?
> 
> This is a scenario different of what is asked above.
> In the case of a specific configuration of one device,
> we have three choices.
> These are your suggestions, with my comments:
> 	1- PMD specific API
> 	2- Extend ethdev dev_ops for each usecase
> 	(3- Have a generic API) = choice 2
> 	(4- Extend configure to accept flags) = choice 1
> This is a choice 3:
> 	- no support of exotic features

Not sure if this is a real option for a vendor, HWs has exotic features and they
want to enable it, I believe SW should not be the blocker for this.

Also I definitely agree that exotic features should not break main/common usage,
make it hard to use or make it confusing/complex etc.

Until we have a better solution I guess we need to continue with private APIs.

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

[Thomas Monjalon]

20/03/2019 11:45, Ferruh Yigit:
> On 3/19/2019 8:30 PM, Thomas Monjalon wrote:
> > 19/03/2019 19:00, Ferruh Yigit:
> >> On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
> >>> 19/03/2019 18:29, Ferruh Yigit:
> >>>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
> >>>>> 14/03/2019 03:58, Hyong Youb Kim:
> >>>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> >>>>>>> 13/03/2019 22:11, John Daley (johndale):
> >>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>> 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.
> >>>>>>>
> >>>>>>> It looks like an application will always set this flag or never.
> >>>>>>> So I don't see the need for an API function.
> >>>>>>> Why VPP prefers set this flag later?
> >>>>>>> Would it be better to have some driver-specific flags, no matter the ports?
> >>>>>>
> >>>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
> >>>>>>
> >>>>>> 1. Leave everything to the user.
> >>>>>>
> >>>>>> Let the human user specify NIC-specific settings (e.g. devarg,
> >>>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
> >>>>>> app simply passes these to DPDK and does nothing else. Devargs are
> >>>>>> passed to rte_eal_init. Other settings are applied during the
> >>>>>> configure phase after rte_eal_init.
> >>>>>>
> >>>>>> For example, OVS seems to go for a smallest common denominator that
> >>>>>> works with most NICs out of the box. Otherwiese, it kinda falls into
> >>>>>> this camp.
> >>>>>>
> >>>>>> For a problematic NIC that needs user intervention, troubleshooting
> >>>>>> goes like this :-)
> >>>>>> - Install app.
> >>>>>> - Run with settings that worked on a previous machine.
> >>>>>> - Some features suddenly do not work.
> >>>>>> - Google search this and that ("why this does not work on this server?").
> >>>>>> - Contact vendors.
> >>>>>> - Find out this NIC has unexpected behavior.
> >>>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
> >>>>>> - Now the features work.
> >>>>>>
> >>>>>> 2. Hide ugly tweaks from the user.
> >>>>>>
> >>>>>> VPP falls into this camp. The user specifies BDFs in the config (no
> >>>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
> >>>>>> discovered ports, applies whatever NIC-specific settings necessary
> >>>>>> during the configure phase (i.e. do this for vendor A NIC, do that for
> >>>>>> vendor B NIC), and then start the ports.
> >>>>>>
> >>>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
> >>>>>> model. One way around it is a private API. Driver specific flags
> >>>>>> during the configure phase would also work fine. Though, enic might be
> >>>>>> the only user of those flags.
> >>>>>
> >>>>> I think DPDK needs some driver configuration.
> >>>>> Currently the config is done per device with devargs.
> >>>>> The next devargs format allow this:
> >>>>> 	driver=enic,rewrite=on
> >>>>> and it can be passed to rte_eal_init().
> >>>>>
> >>>>> We did not progress on the implementation of this format in recent months,
> >>>>> but you are welcome to help!
> >>>>> Instead of passing devargs in the whitelist/blacklist options,
> >>>>> we should introduce a new option, like --dev.
> >>>>
> >>>> But it will be still devarg in new implementation.
> >>>
> >>> With the new syntax, no need to specify a device.
> >>> We can match a driver or multiple drivers sharing the same property.
> >>>
> >>>> I guess for this use case, there is a need to pass this information from an API.
> >>>> Options can be:
> >>>> 1- PMD specific API
> >>>> 2- Extend ethdev dev_ops for each usecase
> >>>> 3- Have a generic API, as suggested above
> >>>> 4- Extend configure to accept flags
> >>>>
> >>>> I don't see a winner in above list, each has some problems. Any comment on how
> >>>> to proceed?
> >>>
> >>> I don't see a problem with the devargs approach.
> >>
> >> Devargs either passed via command line to DPDK application, or parameter to
> >> hotplug APIs.
> > 
> > The application can pass whatever it wants to EAL.
> 
> This means changing current device probe logic completely, right.
> Instead of probing everything on start, probe nothing and application add
> devices via eal (hotplug) API calls with providing devargs.
> I have no issue with this picture, only it doesn't look soon.

No, I mean probe everything at startup automatically as usual.
Just need to pass an option to the driver
during its initialization.

> > In the case described above, the application wants to enable
> > a mode of the driver for all its devices.
> > That's why I think the right solution is a driver option,
> > which can be achieved with the new devargs syntax.
> > 
> >> If someone wants to use regular probe without any command line argument, and
> >> later configure the device via an API, can devargs be used?
> > 
> > This is a scenario different of what is asked above.
> > In the case of a specific configuration of one device,
> > we have three choices.
> > These are your suggestions, with my comments:
> > 	1- PMD specific API
> > 	2- Extend ethdev dev_ops for each usecase
> > 	(3- Have a generic API) = choice 2
> > 	(4- Extend configure to accept flags) = choice 1
> > This is a choice 3:
> > 	- no support of exotic features
> 
> Not sure if this is a real option for a vendor, HWs has exotic features and they
> want to enable it, I believe SW should not be the blocker for this.
> 
> Also I definitely agree that exotic features should not break main/common usage,
> make it hard to use or make it confusing/complex etc.
> 
> Until we have a better solution I guess we need to continue with private APIs.

I think the driver option would work,
but it seems I fail to correctly explain it :)

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

[Ferruh Yigit]

On 3/20/2019 11:46 AM, Thomas Monjalon wrote:
> 20/03/2019 11:45, Ferruh Yigit:
>> On 3/19/2019 8:30 PM, Thomas Monjalon wrote:
>>> 19/03/2019 19:00, Ferruh Yigit:
>>>> On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
>>>>> 19/03/2019 18:29, Ferruh Yigit:
>>>>>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
>>>>>>> 14/03/2019 03:58, Hyong Youb Kim:
>>>>>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
>>>>>>>>> 13/03/2019 22:11, John Daley (johndale):
>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> It looks like an application will always set this flag or never.
>>>>>>>>> So I don't see the need for an API function.
>>>>>>>>> Why VPP prefers set this flag later?
>>>>>>>>> Would it be better to have some driver-specific flags, no matter the ports?
>>>>>>>>
>>>>>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
>>>>>>>>
>>>>>>>> 1. Leave everything to the user.
>>>>>>>>
>>>>>>>> Let the human user specify NIC-specific settings (e.g. devarg,
>>>>>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
>>>>>>>> app simply passes these to DPDK and does nothing else. Devargs are
>>>>>>>> passed to rte_eal_init. Other settings are applied during the
>>>>>>>> configure phase after rte_eal_init.
>>>>>>>>
>>>>>>>> For example, OVS seems to go for a smallest common denominator that
>>>>>>>> works with most NICs out of the box. Otherwiese, it kinda falls into
>>>>>>>> this camp.
>>>>>>>>
>>>>>>>> For a problematic NIC that needs user intervention, troubleshooting
>>>>>>>> goes like this :-)
>>>>>>>> - Install app.
>>>>>>>> - Run with settings that worked on a previous machine.
>>>>>>>> - Some features suddenly do not work.
>>>>>>>> - Google search this and that ("why this does not work on this server?").
>>>>>>>> - Contact vendors.
>>>>>>>> - Find out this NIC has unexpected behavior.
>>>>>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
>>>>>>>> - Now the features work.
>>>>>>>>
>>>>>>>> 2. Hide ugly tweaks from the user.
>>>>>>>>
>>>>>>>> VPP falls into this camp. The user specifies BDFs in the config (no
>>>>>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
>>>>>>>> discovered ports, applies whatever NIC-specific settings necessary
>>>>>>>> during the configure phase (i.e. do this for vendor A NIC, do that for
>>>>>>>> vendor B NIC), and then start the ports.
>>>>>>>>
>>>>>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
>>>>>>>> model. One way around it is a private API. Driver specific flags
>>>>>>>> during the configure phase would also work fine. Though, enic might be
>>>>>>>> the only user of those flags.
>>>>>>>
>>>>>>> I think DPDK needs some driver configuration.
>>>>>>> Currently the config is done per device with devargs.
>>>>>>> The next devargs format allow this:
>>>>>>> 	driver=enic,rewrite=on
>>>>>>> and it can be passed to rte_eal_init().
>>>>>>>
>>>>>>> We did not progress on the implementation of this format in recent months,
>>>>>>> but you are welcome to help!
>>>>>>> Instead of passing devargs in the whitelist/blacklist options,
>>>>>>> we should introduce a new option, like --dev.
>>>>>>
>>>>>> But it will be still devarg in new implementation.
>>>>>
>>>>> With the new syntax, no need to specify a device.
>>>>> We can match a driver or multiple drivers sharing the same property.
>>>>>
>>>>>> I guess for this use case, there is a need to pass this information from an API.
>>>>>> Options can be:
>>>>>> 1- PMD specific API
>>>>>> 2- Extend ethdev dev_ops for each usecase
>>>>>> 3- Have a generic API, as suggested above
>>>>>> 4- Extend configure to accept flags
>>>>>>
>>>>>> I don't see a winner in above list, each has some problems. Any comment on how
>>>>>> to proceed?
>>>>>
>>>>> I don't see a problem with the devargs approach.
>>>>
>>>> Devargs either passed via command line to DPDK application, or parameter to
>>>> hotplug APIs.
>>>
>>> The application can pass whatever it wants to EAL.
>>
>> This means changing current device probe logic completely, right.
>> Instead of probing everything on start, probe nothing and application add
>> devices via eal (hotplug) API calls with providing devargs.
>> I have no issue with this picture, only it doesn't look soon.
> 
> No, I mean probe everything at startup automatically as usual.
> Just need to pass an option to the driver
> during its initialization.
> 
>>> In the case described above, the application wants to enable
>>> a mode of the driver for all its devices.
>>> That's why I think the right solution is a driver option,
>>> which can be achieved with the new devargs syntax.
>>>
>>>> If someone wants to use regular probe without any command line argument, and
>>>> later configure the device via an API, can devargs be used?
>>>
>>> This is a scenario different of what is asked above.
>>> In the case of a specific configuration of one device,
>>> we have three choices.
>>> These are your suggestions, with my comments:
>>> 	1- PMD specific API
>>> 	2- Extend ethdev dev_ops for each usecase
>>> 	(3- Have a generic API) = choice 2
>>> 	(4- Extend configure to accept flags) = choice 1
>>> This is a choice 3:
>>> 	- no support of exotic features
>>
>> Not sure if this is a real option for a vendor, HWs has exotic features and they
>> want to enable it, I believe SW should not be the blocker for this.
>>
>> Also I definitely agree that exotic features should not break main/common usage,
>> make it hard to use or make it confusing/complex etc.
>>
>> Until we have a better solution I guess we need to continue with private APIs.
> 
> I think the driver option would work,
> but it seems I fail to correctly explain it :)
> 

I see it can work if an application always wants this config option to have
*same* value. So it can set this in eal_init() always.

This requires "driver=xxx,key=value" kind of support in devargs.


John, Hyong,

I guess some level of devargs support is already there, Thomas/Gaetan can
provide more information on latest status of it, can it be possible to get some
support from you to finalize this effort?

And when it is ready enic can benefit from it for this usecase.

Thanks,
ferruh

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

[Gaëtan Rivet]

On Mon, Mar 25, 2019 at 11:49:20AM +0000, Ferruh Yigit wrote:
> On 3/20/2019 11:46 AM, Thomas Monjalon wrote:
> > 20/03/2019 11:45, Ferruh Yigit:
> >> On 3/19/2019 8:30 PM, Thomas Monjalon wrote:
> >>> 19/03/2019 19:00, Ferruh Yigit:
> >>>> On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
> >>>>> 19/03/2019 18:29, Ferruh Yigit:
> >>>>>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
> >>>>>>> 14/03/2019 03:58, Hyong Youb Kim:
> >>>>>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> >>>>>>>>> 13/03/2019 22:11, John Daley (johndale):
> >>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>>>> 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.
> >>>>>>>>>
> >>>>>>>>> It looks like an application will always set this flag or never.
> >>>>>>>>> So I don't see the need for an API function.
> >>>>>>>>> Why VPP prefers set this flag later?
> >>>>>>>>> Would it be better to have some driver-specific flags, no matter the ports?
> >>>>>>>>
> >>>>>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
> >>>>>>>>
> >>>>>>>> 1. Leave everything to the user.
> >>>>>>>>
> >>>>>>>> Let the human user specify NIC-specific settings (e.g. devarg,
> >>>>>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
> >>>>>>>> app simply passes these to DPDK and does nothing else. Devargs are
> >>>>>>>> passed to rte_eal_init. Other settings are applied during the
> >>>>>>>> configure phase after rte_eal_init.
> >>>>>>>>
> >>>>>>>> For example, OVS seems to go for a smallest common denominator that
> >>>>>>>> works with most NICs out of the box. Otherwiese, it kinda falls into
> >>>>>>>> this camp.
> >>>>>>>>
> >>>>>>>> For a problematic NIC that needs user intervention, troubleshooting
> >>>>>>>> goes like this :-)
> >>>>>>>> - Install app.
> >>>>>>>> - Run with settings that worked on a previous machine.
> >>>>>>>> - Some features suddenly do not work.
> >>>>>>>> - Google search this and that ("why this does not work on this server?").
> >>>>>>>> - Contact vendors.
> >>>>>>>> - Find out this NIC has unexpected behavior.
> >>>>>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
> >>>>>>>> - Now the features work.
> >>>>>>>>
> >>>>>>>> 2. Hide ugly tweaks from the user.
> >>>>>>>>
> >>>>>>>> VPP falls into this camp. The user specifies BDFs in the config (no
> >>>>>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
> >>>>>>>> discovered ports, applies whatever NIC-specific settings necessary
> >>>>>>>> during the configure phase (i.e. do this for vendor A NIC, do that for
> >>>>>>>> vendor B NIC), and then start the ports.
> >>>>>>>>
> >>>>>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
> >>>>>>>> model. One way around it is a private API. Driver specific flags
> >>>>>>>> during the configure phase would also work fine. Though, enic might be
> >>>>>>>> the only user of those flags.
> >>>>>>>
> >>>>>>> I think DPDK needs some driver configuration.
> >>>>>>> Currently the config is done per device with devargs.
> >>>>>>> The next devargs format allow this:
> >>>>>>> 	driver=enic,rewrite=on
> >>>>>>> and it can be passed to rte_eal_init().
> >>>>>>>
> >>>>>>> We did not progress on the implementation of this format in recent months,
> >>>>>>> but you are welcome to help!
> >>>>>>> Instead of passing devargs in the whitelist/blacklist options,
> >>>>>>> we should introduce a new option, like --dev.
> >>>>>>
> >>>>>> But it will be still devarg in new implementation.
> >>>>>
> >>>>> With the new syntax, no need to specify a device.
> >>>>> We can match a driver or multiple drivers sharing the same property.
> >>>>>
> >>>>>> I guess for this use case, there is a need to pass this information from an API.
> >>>>>> Options can be:
> >>>>>> 1- PMD specific API
> >>>>>> 2- Extend ethdev dev_ops for each usecase
> >>>>>> 3- Have a generic API, as suggested above
> >>>>>> 4- Extend configure to accept flags
> >>>>>>
> >>>>>> I don't see a winner in above list, each has some problems. Any comment on how
> >>>>>> to proceed?
> >>>>>
> >>>>> I don't see a problem with the devargs approach.
> >>>>
> >>>> Devargs either passed via command line to DPDK application, or parameter to
> >>>> hotplug APIs.
> >>>
> >>> The application can pass whatever it wants to EAL.
> >>
> >> This means changing current device probe logic completely, right.
> >> Instead of probing everything on start, probe nothing and application add
> >> devices via eal (hotplug) API calls with providing devargs.
> >> I have no issue with this picture, only it doesn't look soon.
> > 
> > No, I mean probe everything at startup automatically as usual.
> > Just need to pass an option to the driver
> > during its initialization.
> > 
> >>> In the case described above, the application wants to enable
> >>> a mode of the driver for all its devices.
> >>> That's why I think the right solution is a driver option,
> >>> which can be achieved with the new devargs syntax.
> >>>
> >>>> If someone wants to use regular probe without any command line argument, and
> >>>> later configure the device via an API, can devargs be used?
> >>>
> >>> This is a scenario different of what is asked above.
> >>> In the case of a specific configuration of one device,
> >>> we have three choices.
> >>> These are your suggestions, with my comments:
> >>> 	1- PMD specific API
> >>> 	2- Extend ethdev dev_ops for each usecase
> >>> 	(3- Have a generic API) = choice 2
> >>> 	(4- Extend configure to accept flags) = choice 1
> >>> This is a choice 3:
> >>> 	- no support of exotic features
> >>
> >> Not sure if this is a real option for a vendor, HWs has exotic features and they
> >> want to enable it, I believe SW should not be the blocker for this.
> >>
> >> Also I definitely agree that exotic features should not break main/common usage,
> >> make it hard to use or make it confusing/complex etc.
> >>
> >> Until we have a better solution I guess we need to continue with private APIs.
> > 
> > I think the driver option would work,
> > but it seems I fail to correctly explain it :)
> > 
> 
> I see it can work if an application always wants this config option to have
> *same* value. So it can set this in eal_init() always.
> 
> This requires "driver=xxx,key=value" kind of support in devargs.
> 
> 
> John, Hyong,
> 
> I guess some level of devargs support is already there, Thomas/Gaetan can
> provide more information on latest status of it, can it be possible to get some
> support from you to finalize this effort?
> 
> And when it is ready enic can benefit from it for this usecase.
> 
> Thanks,
> ferruh

Hi Thomas, Ferruh, John, Hyong,

driver=xxx,key=value could work, as it is not dependent on the
devargs framework, only on the driver implementation. Nothing specific
should be needed from EAL PoV (regarding this feature only). What will
be missing is the new devargs support in general.

Regarding the new devargs: this dev was stalled 2 versions ago at the
--dev inclusion step. This parameter was necessary for PMD maintainers
to be able to use the new init path with their drivers and transition to
rte_eth_devargs_parse() for devargs parsing.

--dev was proposed, but its patch was not kept by Thomas during the
final crunch. I probably did not shout loud enough about it and let it
go, but I think this was a mistake: this feature was low-risk and central
in the transition process (it was in parallel to -w/b and --vdev).

-- 
Gaëtan Rivet
6WIND

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

[Gaëtan Rivet]

On Mon, Mar 25, 2019 at 02:26:27PM +0100, Gaëtan Rivet wrote:
> On Mon, Mar 25, 2019 at 11:49:20AM +0000, Ferruh Yigit wrote:
> > On 3/20/2019 11:46 AM, Thomas Monjalon wrote:
> > > 20/03/2019 11:45, Ferruh Yigit:
> > >> On 3/19/2019 8:30 PM, Thomas Monjalon wrote:
> > >>> 19/03/2019 19:00, Ferruh Yigit:
> > >>>> On 3/19/2019 5:36 PM, Thomas Monjalon wrote:
> > >>>>> 19/03/2019 18:29, Ferruh Yigit:
> > >>>>>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote:
> > >>>>>>> 14/03/2019 03:58, Hyong Youb Kim:
> > >>>>>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote:
> > >>>>>>>>> 13/03/2019 22:11, John Daley (johndale):
> > >>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>>>>>>>>> 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.
> > >>>>>>>>>
> > >>>>>>>>> It looks like an application will always set this flag or never.
> > >>>>>>>>> So I don't see the need for an API function.
> > >>>>>>>>> Why VPP prefers set this flag later?
> > >>>>>>>>> Would it be better to have some driver-specific flags, no matter the ports?
> > >>>>>>>>
> > >>>>>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks.
> > >>>>>>>>
> > >>>>>>>> 1. Leave everything to the user.
> > >>>>>>>>
> > >>>>>>>> Let the human user specify NIC-specific settings (e.g. devarg,
> > >>>>>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The
> > >>>>>>>> app simply passes these to DPDK and does nothing else. Devargs are
> > >>>>>>>> passed to rte_eal_init. Other settings are applied during the
> > >>>>>>>> configure phase after rte_eal_init.
> > >>>>>>>>
> > >>>>>>>> For example, OVS seems to go for a smallest common denominator that
> > >>>>>>>> works with most NICs out of the box. Otherwiese, it kinda falls into
> > >>>>>>>> this camp.
> > >>>>>>>>
> > >>>>>>>> For a problematic NIC that needs user intervention, troubleshooting
> > >>>>>>>> goes like this :-)
> > >>>>>>>> - Install app.
> > >>>>>>>> - Run with settings that worked on a previous machine.
> > >>>>>>>> - Some features suddenly do not work.
> > >>>>>>>> - Google search this and that ("why this does not work on this server?").
> > >>>>>>>> - Contact vendors.
> > >>>>>>>> - Find out this NIC has unexpected behavior.
> > >>>>>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for ..").
> > >>>>>>>> - Now the features work.
> > >>>>>>>>
> > >>>>>>>> 2. Hide ugly tweaks from the user.
> > >>>>>>>>
> > >>>>>>>> VPP falls into this camp. The user specifies BDFs in the config (no
> > >>>>>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the
> > >>>>>>>> discovered ports, applies whatever NIC-specific settings necessary
> > >>>>>>>> during the configure phase (i.e. do this for vendor A NIC, do that for
> > >>>>>>>> vendor B NIC), and then start the ports.
> > >>>>>>>>
> > >>>>>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this
> > >>>>>>>> model. One way around it is a private API. Driver specific flags
> > >>>>>>>> during the configure phase would also work fine. Though, enic might be
> > >>>>>>>> the only user of those flags.
> > >>>>>>>
> > >>>>>>> I think DPDK needs some driver configuration.
> > >>>>>>> Currently the config is done per device with devargs.
> > >>>>>>> The next devargs format allow this:
> > >>>>>>> 	driver=enic,rewrite=on
> > >>>>>>> and it can be passed to rte_eal_init().
> > >>>>>>>
> > >>>>>>> We did not progress on the implementation of this format in recent months,
> > >>>>>>> but you are welcome to help!
> > >>>>>>> Instead of passing devargs in the whitelist/blacklist options,
> > >>>>>>> we should introduce a new option, like --dev.
> > >>>>>>
> > >>>>>> But it will be still devarg in new implementation.
> > >>>>>
> > >>>>> With the new syntax, no need to specify a device.
> > >>>>> We can match a driver or multiple drivers sharing the same property.
> > >>>>>
> > >>>>>> I guess for this use case, there is a need to pass this information from an API.
> > >>>>>> Options can be:
> > >>>>>> 1- PMD specific API
> > >>>>>> 2- Extend ethdev dev_ops for each usecase
> > >>>>>> 3- Have a generic API, as suggested above
> > >>>>>> 4- Extend configure to accept flags
> > >>>>>>
> > >>>>>> I don't see a winner in above list, each has some problems. Any comment on how
> > >>>>>> to proceed?
> > >>>>>
> > >>>>> I don't see a problem with the devargs approach.
> > >>>>
> > >>>> Devargs either passed via command line to DPDK application, or parameter to
> > >>>> hotplug APIs.
> > >>>
> > >>> The application can pass whatever it wants to EAL.
> > >>
> > >> This means changing current device probe logic completely, right.
> > >> Instead of probing everything on start, probe nothing and application add
> > >> devices via eal (hotplug) API calls with providing devargs.
> > >> I have no issue with this picture, only it doesn't look soon.
> > > 
> > > No, I mean probe everything at startup automatically as usual.
> > > Just need to pass an option to the driver
> > > during its initialization.
> > > 
> > >>> In the case described above, the application wants to enable
> > >>> a mode of the driver for all its devices.
> > >>> That's why I think the right solution is a driver option,
> > >>> which can be achieved with the new devargs syntax.
> > >>>
> > >>>> If someone wants to use regular probe without any command line argument, and
> > >>>> later configure the device via an API, can devargs be used?
> > >>>
> > >>> This is a scenario different of what is asked above.
> > >>> In the case of a specific configuration of one device,
> > >>> we have three choices.
> > >>> These are your suggestions, with my comments:
> > >>> 	1- PMD specific API
> > >>> 	2- Extend ethdev dev_ops for each usecase
> > >>> 	(3- Have a generic API) = choice 2
> > >>> 	(4- Extend configure to accept flags) = choice 1
> > >>> This is a choice 3:
> > >>> 	- no support of exotic features
> > >>
> > >> Not sure if this is a real option for a vendor, HWs has exotic features and they
> > >> want to enable it, I believe SW should not be the blocker for this.
> > >>
> > >> Also I definitely agree that exotic features should not break main/common usage,
> > >> make it hard to use or make it confusing/complex etc.
> > >>
> > >> Until we have a better solution I guess we need to continue with private APIs.
> > > 
> > > I think the driver option would work,
> > > but it seems I fail to correctly explain it :)
> > > 
> > 
> > I see it can work if an application always wants this config option to have
> > *same* value. So it can set this in eal_init() always.
> > 
> > This requires "driver=xxx,key=value" kind of support in devargs.
> > 
> > 
> > John, Hyong,
> > 
> > I guess some level of devargs support is already there, Thomas/Gaetan can
> > provide more information on latest status of it, can it be possible to get some
> > support from you to finalize this effort?
> > 
> > And when it is ready enic can benefit from it for this usecase.
> > 
> > Thanks,
> > ferruh
> 
> Hi Thomas, Ferruh, John, Hyong,
> 
> driver=xxx,key=value could work, as it is not dependent on the
> devargs framework, only on the driver implementation. Nothing specific
> should be needed from EAL PoV (regarding this feature only). What will
> be missing is the new devargs support in general.

Sorry I spoke too quickly, specific development and some passing of
arguments would be needed.

> 
> Regarding the new devargs: this dev was stalled 2 versions ago at the
> --dev inclusion step. This parameter was necessary for PMD maintainers
> to be able to use the new init path with their drivers and transition to
> rte_eth_devargs_parse() for devargs parsing.
> 
> --dev was proposed, but its patch was not kept by Thomas during the
> final crunch. I probably did not shout loud enough about it and let it
> go, but I think this was a mistake: this feature was low-risk and central
> in the transition process (it was in parallel to -w/b and --vdev).
> 
> -- 
> Gaëtan Rivet
> 6WIND

-- 
Gaëtan Rivet
6WIND

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

[Hyong Youb Kim]

[...]

> > > I see it can work if an application always wants this config option to have
> > > *same* value. So it can set this in eal_init() always.
> > > 
> > > This requires "driver=xxx,key=value" kind of support in devargs.
> > > 
> > > 
> > > John, Hyong,
> > > 
> > > I guess some level of devargs support is already there, Thomas/Gaetan can
> > > provide more information on latest status of it, can it be possible to get some
> > > support from you to finalize this effort?
> > > 
> > > And when it is ready enic can benefit from it for this usecase.
> > > 
> > > Thanks,
> > > ferruh
> > 
> > Hi Thomas, Ferruh, John, Hyong,
> > 
> > driver=xxx,key=value could work, as it is not dependent on the
> > devargs framework, only on the driver implementation. Nothing specific
> > should be needed from EAL PoV (regarding this feature only). What will
> > be missing is the new devargs support in general.
> 
> Sorry I spoke too quickly, specific development and some passing of
> arguments would be needed.
> 
> > 
> > Regarding the new devargs: this dev was stalled 2 versions ago at the
> > --dev inclusion step. This parameter was necessary for PMD maintainers
> > to be able to use the new init path with their drivers and transition to
> > rte_eth_devargs_parse() for devargs parsing.
> > 
> > --dev was proposed, but its patch was not kept by Thomas during the
> > final crunch. I probably did not shout loud enough about it and let it
> > go, but I think this was a mistake: this feature was low-risk and central
> > in the transition process (it was in parallel to -w/b and --vdev).

Hi All,

Thanks for tracking this issue. Yes, we will work on "driver=x,k=v"
and try to submit patches early in the 19.08 cycle. Cannot keep
free-riding :-)

As for vpp, it can have its own local DPDK patches. So, we added an
enic patch to change the default rewrite mode to 'untag', which has
the same effect as 'driver=enic,rewrite=untag'.

-Hyong