[v3] net/i40e: disable source pruning

Message ID 20211020012831.8480-1-alvinx.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series [v3] net/i40e: disable source pruning |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-spell-check-testing warning Testing issues
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Alvin Zhang Oct. 20, 2021, 1:28 a.m. UTC
  VRRP advertisement packets are dropped on i40e PF devices because
when a MAC address is added to a device, packets originating from
that MAC address are dropped.

This patch adds a devarg to support disabling source pruning to work
around above issue.

Bugzilla ID: 648

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

v3: Rebase to 69a3c6319140. Add bugzilla ID in commit log.
---
 doc/guides/nics/i40e.rst       |  9 ++++++
 drivers/net/i40e/i40e_ethdev.c | 65 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h |  1 +
 3 files changed, 75 insertions(+)
  

Comments

Yu Jiang Feb. 21, 2022, 8:30 a.m. UTC | #1
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> Sent: Wednesday, October 20, 2021 9:29 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng
> <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning
> 
> VRRP advertisement packets are dropped on i40e PF devices because when
> a MAC address is added to a device, packets originating from that MAC
> address are dropped.
> 
> This patch adds a devarg to support disabling source pruning to work around
> above issue.
> 
> Bugzilla ID: 648
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
Tested-by:  Yu Jiang <YuX.Jiang@intel.com>

Verified patchset http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e "raw/cnxk_gpio: add option to select subset of GPIOs"
Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS: Fedora Linux 35/5.14.10-300.fc35.x86_64
Test step as below:
 ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 -- -i
 pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
 test steps:
1). testpmd>set verbose 1
    testpmd>start
2). Send the pkt, the pkt can be received by testpmd
3). testpmd>mac_addr add 0 00:00:5E:00:01:0A 
4). Re-send the pkt, the pkt still can be received by testpmd.
  
Morten Brørup Feb. 21, 2022, 9:35 a.m. UTC | #2
> From: Jiang, YuX [mailto:yux.jiang@intel.com]
> Sent: Monday, 21 February 2022 09.31
> 
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > Sent: Wednesday, October 20, 2021 9:29 AM
> >
> > VRRP advertisement packets are dropped on i40e PF devices because
> when
> > a MAC address is added to a device, packets originating from that MAC
> > address are dropped.
> >
> > This patch adds a devarg to support disabling source pruning to work
> around
> > above issue.
> >
> > Bugzilla ID: 648
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> 
> Verified patchset
> http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> "raw/cnxk_gpio: add option to select subset of GPIOs"
> Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> Fedora Linux 35/5.14.10-300.fc35.x86_64
> Test step as below:
>  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 -- -i
>  pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
>  test steps:
> 1). testpmd>set verbose 1
>     testpmd>start
> 2). Send the pkt, the pkt can be received by testpmd
> 3). testpmd>mac_addr add 0 00:00:5E:00:01:0A
> 4). Re-send the pkt, the pkt still can be received by testpmd.

If source pruning is not the default behavior of all NICs, it should be disabled by default in the i40e NIC too.

A NIC shouldn't drop any packets unless it has explicitly been configured for it! And a NIC shouldn't treat any packets differently than other NICs do, unless the NIC has explicitly been configured so!

Furthermore, I would prefer that configurations for explicitly dropping certain types of packets is available through runtime APIs, e.g. RTE_FLOWS, or dedicated functions like rte_eth_promiscuous_enable/disable(). This patch doesn't support runtime detection of installed NICs performed by the application.

I am very surprised by this default behavior of a NIC. Please confirm that Source Pruning is at least disabled in Promiscuous mode?

-Morten
  
Zhang, Ke1X Dec. 26, 2022, 9:03 a.m. UTC | #3
> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, February 21, 2022 5:35 PM
> To: Jiang, YuX <yux.jiang@intel.com>; Alvin Zhang <alvinx.zhang@intel.com>;
> Xing, Beilei <beilei.xing@intel.com>; Guo, Junfeng <junfeng.guo@intel.com>
> Cc: dev@dpdk.org; Zhang, AlvinX <alvinx.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3] net/i40e: disable source pruning
> 
> > From: Jiang, YuX [mailto:yux.jiang@intel.com]
> > Sent: Monday, 21 February 2022 09.31
> >
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > > Sent: Wednesday, October 20, 2021 9:29 AM
> > >
> > > VRRP advertisement packets are dropped on i40e PF devices because
> > when
> > > a MAC address is added to a device, packets originating from that
> > > MAC address are dropped.
> > >
> > > This patch adds a devarg to support disabling source pruning to work
> > around
> > > above issue.
> > >
> > > Bugzilla ID: 648
> > >
> > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > ---
> > Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> >
> > Verified patchset
> > http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> > alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> > "raw/cnxk_gpio: add option to select subset of GPIOs"
> > Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> > Fedora Linux 35/5.14.10-300.fc35.x86_64 Test step as below:
> >  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 -- -i
> > pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
> >  test steps:
> > 1). testpmd>set verbose 1
> >     testpmd>start
> > 2). Send the pkt, the pkt can be received by testpmd 3).
> > testpmd>mac_addr add 0 00:00:5E:00:01:0A 4). Re-send the pkt, the pkt
> > still can be received by testpmd.
> 
> If source pruning is not the default behavior of all NICs, it should be disabled
> by default in the i40e NIC too.
> 
> A NIC shouldn't drop any packets unless it has explicitly been configured for it!
> And a NIC shouldn't treat any packets differently than other NICs do, unless
> the NIC has explicitly been configured so!
> 
> Furthermore, I would prefer that configurations for explicitly dropping
> certain types of packets is available through runtime APIs, e.g. RTE_FLOWS,
> or dedicated functions like rte_eth_promiscuous_enable/disable(). This
> patch doesn't support runtime detection of installed NICs performed by the
> application.
> 
> I am very surprised by this default behavior of a NIC. Please confirm that
> Source Pruning is at least disabled in Promiscuous mode?
> 
> -Morten

Thanks for your comments @Morten Brørup
After testing with other NIC like ice, source pruning is the default action, it means it is the same action
for both ice and i40e when receiving in default.
In this patch, the default is "not disable", it is same with other NICs.
  
Morten Brørup Dec. 26, 2022, 10:26 a.m. UTC | #4
+CC Ethernet API maintainers, you might have an opinion about expected default behavior

> From: Zhang, Ke1X [mailto:ke1x.zhang@intel.com]
> Sent: Monday, 26 December 2022 10.04
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Monday, February 21, 2022 5:35 PM
> >
> > > From: Jiang, YuX [mailto:yux.jiang@intel.com]
> > > Sent: Monday, 21 February 2022 09.31
> > >
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > > > Sent: Wednesday, October 20, 2021 9:29 AM
> > > >
> > > > VRRP advertisement packets are dropped on i40e PF devices because
> > > when
> > > > a MAC address is added to a device, packets originating from that
> > > > MAC address are dropped.
> > > >
> > > > This patch adds a devarg to support disabling source pruning to
> work
> > > around
> > > > above issue.
> > > >
> > > > Bugzilla ID: 648
> > > >
> > > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > > ---
> > > Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> > >
> > > Verified patchset
> > > http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> > > alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> > > "raw/cnxk_gpio: add option to select subset of GPIOs"
> > > Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> > > Fedora Linux 35/5.14.10-300.fc35.x86_64 Test step as below:
> > >  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 --
> -i
> > > pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
> > >  test steps:
> > > 1). testpmd>set verbose 1
> > >     testpmd>start
> > > 2). Send the pkt, the pkt can be received by testpmd 3).
> > > testpmd>mac_addr add 0 00:00:5E:00:01:0A 4). Re-send the pkt, the
> pkt
> > > still can be received by testpmd.
> >
> > If source pruning is not the default behavior of all NICs, it should
> be disabled
> > by default in the i40e NIC too.
> >
> > A NIC shouldn't drop any packets unless it has explicitly been
> configured for it!
> > And a NIC shouldn't treat any packets differently than other NICs do,
> unless
> > the NIC has explicitly been configured so!
> >
> > Furthermore, I would prefer that configurations for explicitly
> dropping
> > certain types of packets is available through runtime APIs, e.g.
> RTE_FLOWS,
> > or dedicated functions like rte_eth_promiscuous_enable/disable().
> This
> > patch doesn't support runtime detection of installed NICs performed
> by the
> > application.
> >
> > I am very surprised by this default behavior of a NIC. Please confirm
> that
> > Source Pruning is at least disabled in Promiscuous mode?
> >
> > -Morten
> 
> Thanks for your comments @Morten Brørup
> After testing with other NIC like ice, source pruning is the default
> action, it means it is the same action
> for both ice and i40e when receiving in default.

I meant NICs in general, not just Intel NICs. Is it standard behavior for NICs in general (incl. NVIDIA, Broadcom, etc.) to perform source pruning?

They way I read the bug report, the source pruning behavior is specific to Intel NICs. Applications should not be required to pass specific command line parameters to make one vendor's PMDs behave like the general behavior of PMDs from other vendors.

> In this patch, the default is "not disable", it is same with other
> NICs.

Not other NICs in general, only other Intel NICs.

I acknowledge that this patch is a workaround, and certainly an improvement, but why not provide a fix instead? It could be backported to older DPDK releases.

If you want to stick with this workaround instead of providing a proper fix, please also update the documentation to mention that disable_source_pruning=1 is required.

Please also describe how to disable source pruning at runtime. Some applications detect the installed NICs at runtime, so passing NIC specific parameters to the application at startup is not possible.

-Morten
  
Morten Brørup Dec. 26, 2022, 10:27 a.m. UTC | #5
> From: Zhang, Ke1X [mailto:ke1x.zhang@intel.com]
> Sent: Monday, 26 December 2022 10.04
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Monday, February 21, 2022 5:35 PM
> >
> > > From: Jiang, YuX [mailto:yux.jiang@intel.com]
> > > Sent: Monday, 21 February 2022 09.31
> > >
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Alvin Zhang
> > > > Sent: Wednesday, October 20, 2021 9:29 AM
> > > >
> > > > VRRP advertisement packets are dropped on i40e PF devices because
> > > when
> > > > a MAC address is added to a device, packets originating from that
> > > > MAC address are dropped.
> > > >
> > > > This patch adds a devarg to support disabling source pruning to
> work
> > > around
> > > > above issue.
> > > >
> > > > Bugzilla ID: 648
> > > >
> > > > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > > > ---
> > > Tested-by:  Yu Jiang <YuX.Jiang@intel.com>
> > >
> > > Verified patchset
> > > http://patches.dpdk.org/project/dpdk/patch/20211020012831.8480-1-
> > > alvinx.zhang@intel.com/ on baseline dpdk22.03-rc1:ecc0dd455e
> > > "raw/cnxk_gpio: add option to select subset of GPIOs"
> > > Tested pass on Ethernet Controller XL710 for 40GbE QSFP+ 1583, OS:
> > > Fedora Linux 35/5.14.10-300.fc35.x86_64 Test step as below:
> > >  ./dpdk-testpmd -l 1,2 -n 1 -a 18:00.0,disable_source_pruning=1 --
> -i
> > > pkt = Ether(src="00:00:5E:00:01:0A")/IP()/Raw("x"*60)
> > >  test steps:
> > > 1). testpmd>set verbose 1
> > >     testpmd>start
> > > 2). Send the pkt, the pkt can be received by testpmd 3).
> > > testpmd>mac_addr add 0 00:00:5E:00:01:0A 4). Re-send the pkt, the
> pkt
> > > still can be received by testpmd.
> >
> > If source pruning is not the default behavior of all NICs, it should
> be disabled
> > by default in the i40e NIC too.
> >
> > A NIC shouldn't drop any packets unless it has explicitly been
> configured for it!
> > And a NIC shouldn't treat any packets differently than other NICs do,
> unless
> > the NIC has explicitly been configured so!
> >
> > Furthermore, I would prefer that configurations for explicitly
> dropping
> > certain types of packets is available through runtime APIs, e.g.
> RTE_FLOWS,
> > or dedicated functions like rte_eth_promiscuous_enable/disable().
> This
> > patch doesn't support runtime detection of installed NICs performed
> by the
> > application.
> >
> > I am very surprised by this default behavior of a NIC. Please confirm
> that
> > Source Pruning is at least disabled in Promiscuous mode?
> >
> > -Morten
> 
> Thanks for your comments @Morten Brørup
> After testing with other NIC like ice, source pruning is the default
> action, it means it is the same action
> for both ice and i40e when receiving in default.

I meant NICs in general, not just Intel NICs. Is it standard behavior for NICs in general (incl. NVIDIA, Broadcom, etc.) to perform source pruning?

They way I read the bug report, the source pruning behavior is specific to Intel NICs. Applications should not be required to pass specific command line parameters to make one vendor's PMDs behave like the general behavior of PMDs from other vendors.

> In this patch, the default is "not disable", it is same with other
> NICs.

Not other NICs in general, only other Intel NICs.

I acknowledge that this patch is a workaround, and certainly an improvement, but why not provide a fix instead? It could be backported to older DPDK releases.

If you want to stick with this workaround instead of providing a proper fix, please also update the documentation to mention that disable_source_pruning=1 is required.

Please also describe how to disable source pruning at runtime. Some applications detect the installed NICs at runtime, so passing NIC specific parameters to the application at startup is not possible.

-Morten
  

Patch

diff --git a/doc/guides/nics/i40e.rst b/doc/guides/nics/i40e.rst
index ef91b3a..1089a3a 100644
--- a/doc/guides/nics/i40e.rst
+++ b/doc/guides/nics/i40e.rst
@@ -236,6 +236,15 @@  Runtime Config Options
 
   -a 84:00.0,vf_msg_cfg=80@120:180
 
+- ``Disable source pruning on PF`` (default ``not disabled``)
+
+  When a MAC address is added to a device, packets originating from that MAC
+  address will be dropped for source pruning. To disable source pruning on PF
+  we can add ``devargs`` parameter ``disable_source_pruning=[0,1]`` and set its
+  value to 1, for example::
+
+  -a 84:00.0,disable_source_pruning=1
+
 Vector RX Pre-conditions
 ~~~~~~~~~~~~~~~~~~~~~~~~
 For Vector RX it is assumed that the number of descriptor rings will be a power
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 1fc3d89..3722e87 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -47,6 +47,7 @@ 
 #define ETH_I40E_SUPPORT_MULTI_DRIVER	"support-multi-driver"
 #define ETH_I40E_QUEUE_NUM_PER_VF_ARG	"queue-num-per-vf"
 #define ETH_I40E_VF_MSG_CFG		"vf_msg_cfg"
+#define ETH_I40E_DISABLE_SOURCE_PRUNING_ARG	"disable_source_pruning"
 
 #define I40E_CLEAR_PXE_WAIT_MS     200
 #define I40E_VSI_TSR_QINQ_STRIP		0x4010
@@ -411,6 +412,7 @@  static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 	ETH_I40E_SUPPORT_MULTI_DRIVER,
 	ETH_I40E_QUEUE_NUM_PER_VF_ARG,
 	ETH_I40E_VF_MSG_CFG,
+	ETH_I40E_DISABLE_SOURCE_PRUNING_ARG,
 	NULL};
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -1368,6 +1370,23 @@  static inline void i40e_clear_automask(struct i40e_pf *pf)
 }
 
 static int
+i40e_parse_bool(__rte_unused const char *key, const char *value, void *opaque)
+{
+	char *end;
+	unsigned long i;
+
+	errno = 0;
+	i = strtoul(value, &end, 10);
+	if (errno != 0 || end == value || *end != 0 || i > 1) {
+		PMD_DRV_LOG(ERR, "Wrong bool value: %s", value);
+		return -EINVAL;
+	}
+
+	*(bool *)opaque = (bool)i;
+	return 0;
+}
+
+static int
 i40e_parse_vf_msg_config(struct rte_eth_dev *dev,
 		struct i40e_vf_msg_cfg *msg_cfg)
 {
@@ -1404,6 +1423,42 @@  static inline void i40e_clear_automask(struct i40e_pf *pf)
 	return ret;
 }
 
+static int
+i40e_parse_source_pruning_config(struct rte_eth_dev *dev)
+{
+	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
+	struct rte_kvargs *kvlist;
+	int kvargs_count;
+	int ret = -EINVAL;
+
+	if (!dev->device->devargs)
+		return 0;
+
+	kvlist = rte_kvargs_parse(dev->device->devargs->args, valid_keys);
+	if (!kvlist)
+		return 0;
+
+	kvargs_count = rte_kvargs_count(kvlist,
+					ETH_I40E_DISABLE_SOURCE_PRUNING_ARG);
+	if (kvargs_count > 1) {
+		PMD_DRV_LOG(ERR, "More than one argument \"%s\"!",
+			    ETH_I40E_DISABLE_SOURCE_PRUNING_ARG);
+		goto free_end;
+	}
+
+	if (kvargs_count &&
+	    rte_kvargs_process(kvlist, ETH_I40E_DISABLE_SOURCE_PRUNING_ARG,
+			       i40e_parse_bool,
+			       &pf->disable_source_pruning) < 0)
+		goto free_end;
+
+	ret = 0;
+
+free_end:
+	rte_kvargs_free(kvlist);
+	return ret;
+}
+
 #define I40E_ALARM_INTERVAL 50000 /* us */
 
 static int
@@ -1487,6 +1542,10 @@  static inline void i40e_clear_automask(struct i40e_pf *pf)
 	/* Check if need to support multi-driver */
 	i40e_support_multi_driver(dev);
 
+	ret = i40e_parse_source_pruning_config(dev);
+	if (ret)
+		return ret;
+
 	/* Make sure all is clean before doing PF reset */
 	i40e_clear_hw(hw);
 
@@ -5812,6 +5871,12 @@  struct i40e_vsi *
 		ctxt.pf_num = hw->pf_id;
 		ctxt.uplink_seid = vsi->uplink_seid;
 		ctxt.vf_num = 0;
+		if (pf->disable_source_pruning) {
+			ctxt.info.valid_sections |=
+				cpu_to_le16(I40E_AQ_VSI_PROP_SWITCH_VALID);
+			ctxt.info.switch_id |=
+				cpu_to_le16(I40E_AQ_VSI_SW_ID_FLAG_LOCAL_LB);
+		}
 
 		/* Update VSI parameters */
 		ret = i40e_aq_update_vsi_params(hw, &ctxt, NULL);
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 8a68b36..b441153 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -1171,6 +1171,7 @@  struct i40e_pf {
 	bool dport_replace_flag;   /* Destination port replace is done */
 	struct i40e_tm_conf tm_conf;
 	bool support_multi_driver; /* 1 - support multiple driver */
+	bool disable_source_pruning; /* 1 - disable source pruning */
 
 	/* Dynamic Device Personalization */
 	bool gtp_support; /* 1 - support GTP-C and GTP-U */