[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
2016-11-10 13:50, Mori, Naoyuki:
> Hi,
>
> >Thomas wrote:
> > > Just to make it sure, you mean returning an error in the driver when
> > > a configuration cannot be applied, right?
> >
> >Yes, as in 1bbcc5d21129 ("i40evf: report error for unsupported CRC
> >stripping config"), where -EINVAL is returned.
> >
> >Bj?rn
>
> On my experience, OvS+DPDK has same issue.
> You cannot run OvS on i40evf due to this CRC config mismatch, while OvS on
> ixgbevf works fine.
> So, changing on i40evf PMD side would have more benefit, I believe.
No I think OVS should handle the configuration error.
We could also add a function to query this capability before configuring.
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Hi Thomas,
> 2016-11-10 13:50, Mori, Naoyuki:
> > >Thomas wrote:
> > > > Just to make it sure, you mean returning an error in the driver when
> > > > a configuration cannot be applied, right?
> > >
> > >Yes, as in 1bbcc5d21129 ("i40evf: report error for unsupported CRC
> > >stripping config"), where -EINVAL is returned.
> > >
> > >Bj?rn
> >
> > On my experience, OvS+DPDK has same issue.
> > You cannot run OvS on i40evf due to this CRC config mismatch, while OvS on
> ixgbevf works fine.
> > So, changing on i40evf PMD side would have more benefit, I believe.
>
> No I think OVS should handle the configuration error.
> We could also add a function to query this capability before configuring.
That would be fine, as long as it became a well-known standard.
But at least, hope i40evf and ixgbevf to be consistent in this CRC handling.
Thanks,
Mori
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Hi,
Re:
>Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping
> for i40evf
>Message-ID:
>Thomas wrote:
> > Just to make it sure, you mean returning an error in the driver when
> > a configuration cannot be applied, right?
>
>Yes, as in 1bbcc5d21129 ("i40evf: report error for unsupported CRC
>stripping config"), where -EINVAL is returned.
>
>Bj?rn
On my experience, OvS+DPDK has same issue.
You cannot run OvS on i40evf due to this CRC config mismatch, while OvS on
ixgbevf works fine.
So, changing on i40evf PMD side would have more benefit, I believe.
[Details below]
ovs-vswitchd.log:
2016-11-10T08:53:31.290Z|00054|netdev_dpdk|WARN|Interface dpdk0 eth_dev setup
error Invalid argument
because of
i40evf_dev_configure() returns ?EINVAL.
At here
---
i40evf_dev_configure(struct rte_eth_dev *dev)
{
/* For non-DPDK PF drivers, VF has no ability to disable HW
* CRC strip, and is implicitly enabled by the PF.
*/
if (!conf->rxmode.hw_strip_crc) {
vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
if ((vf->version_major == I40E_VIRTCHNL_VERSION_MAJOR) &&
(vf->version_minor <= I40E_VIRTCHNL_VERSION_MINOR)) {
/* Peer is running non-DPDK PF driver. */
PMD_INIT_LOG(ERR, "VF can't disable HW CRC Strip");
return -EINVAL;
}
}
}
---
ixgbevf is same Intel NIC but implementation is different. It won?t return
error.
ixgbevf_dev_configure(struct rte_eth_dev *dev)
{
/*
* VF has no ability to enable/disable HW CRC
* Keep the persistent behavior the same as Host PF
*/
#ifndef RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC
if (!conf->rxmode.hw_strip_crc) {
PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
conf->rxmode.hw_strip_crc = 1;
}
#else
if (conf->rxmode.hw_strip_crc) {
PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
conf->rxmode.hw_strip_crc = 0;
}
#endif
}
Regards and Thanks,
Mori
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Thomas wrote:
> Just to make it sure, you mean returning an error in the driver when
> a configuration cannot be applied, right?
Yes, as in 1bbcc5d21129 ("i40evf: report error for unsupported CRC
stripping config"), where -EINVAL is returned.
Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
2016-11-10 07:17, Bj?rn T?pel: > As discussed in the thread, it might be better to just change the > default in l3fwd from .hw_strip_crc = 0 to 1. > > I'll be looking into changing igbvf and ixgbevf to match the semantics > of i40evf. Just to make it sure, you mean returning an error in the driver when a configuration cannot be applied, right?
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Lei wrote: > I'm testing some DPDK sample under VMware. During the testing work, I > find l3fwd+ ixgbe vf can work ,but L3fwd + i40evf can't work. So I > reported this issue to Bjorn. From my perspective, if can add new > parameter in l3fwd sample like what have already don?t in testpmd > "crc-strip enable" is a better way to resolve this issue. (Please don't top-post.) As discussed in the thread, it might be better to just change the default in l3fwd from .hw_strip_crc = 0 to 1. I'll be looking into changing igbvf and ixgbevf to match the semantics of i40evf. Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
I'm testing some DPDK sample under VMware. During the testing work, I find l3fwd+ ixgbe vf can work ,but L3fwd + i40evf can't work. So I reported this issue to Bjorn. From my perspective, if can add new parameter in l3fwd sample like what have already don?t in testpmd "crc-strip enable" is a better way to resolve this issue. Lei -Original Message- From: Topel, Bjorn Sent: Wednesday, November 9, 2016 9:10 PM To: Zhang, Helin ; Ananyev, Konstantin ; dev at dpdk.org Cc: Xu, Qian Q ; Yao, Lei A ; Wu, Jingjing ; thomas.monjalon at 6wind.com Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf Bj?rn/Konstantin wrote: >> Finally, why doesn't l3fwd have the CRC stripped? > > I don?t know any good reason for that for l3fwd or any other sample > app. I think it is just a 'historical' reason. Ok! Then I'd suggest changing the l3fwd default to actually *strip* CRC instead of not doing it. Lei, any comments? Helin wrote: > Yes, i40e driver changed a little bit on that according to the review > comments during implementation, comparing to igb and ixgbe. > I'd suggest to re-invesitgate if we can do the similar thing in igb > and ixgbe driver. Good. Let's do that! > Any critical issue now? Or just an improvement comments? Not from my perspective. The issue is that Lei needs some kind of work-around for l3fwd with i40evf, so I'll let Lei comment on how critical it is. Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Bj?rn/Konstantin wrote: >> Finally, why doesn't l3fwd have the CRC stripped? > > I don?t know any good reason for that for l3fwd or any other sample > app. I think it is just a 'historical' reason. Ok! Then I'd suggest changing the l3fwd default to actually *strip* CRC instead of not doing it. Lei, any comments? Helin wrote: > Yes, i40e driver changed a little bit on that according to the > review comments during implementation, comparing to igb and ixgbe. > I'd suggest to re-invesitgate if we can do the similar thing in igb > and ixgbe driver. Good. Let's do that! > Any critical issue now? Or just an improvement comments? Not from my perspective. The issue is that Lei needs some kind of work-around for l3fwd with i40evf, so I'll let Lei comment on how critical it is. Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
>> Correct, so the broader question would be "what is the correct >> behavior for an example application, when a port configuration >> isn't supported by the hardware?". >> >> My stand, FWIW, is that igb and ixgbe should have the same >> semantics as i40e currently has, i.e. return an error to the user >> if the port is mis-configured, NOT changing the setting behind the >> users back. >> > > Fine by me, but then it means that the fix haw to include changes > for all apps plus ixgbe and igb PMDs, correct? :) Ugh. Correct, I guess. :-) As for ixgbe and igb - they need a patch changing from silent ignore to explicit error. Regarding the apps, I guess all the apps that rely on that disabling CRC stripping always work, need some work. Or should all the example applications have CRC stripping *enabled* by default? I'd assume that all DPDK supported NICs has support for CRC stripping and I guess this is the rational for having it on by default for Intel VFs. In general, for the example applications, if an application relies on a property for a port, that the hardware doesn't support -- what would be the desired behavior? Or is it implied that the example applications only use a common, minimal set of features that are know to be supported by all DPDK supported hardware? Isn't it perfectly valid that some example applications wont run for all hardware? Finally, why doesn't l3fwd have the CRC stripped? Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
> -Original Message- > From: Topel, Bjorn > Sent: Wednesday, November 9, 2016 11:28 AM > To: Ananyev, Konstantin ; dev at dpdk.org; > Zhang, Helin > Cc: Xu, Qian Q ; Yao, Lei A ; > Wu, Jingjing ; > thomas.monjalon at 6wind.com > Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf > > >> Correct, so the broader question would be "what is the correct > >> behavior for an example application, when a port configuration > >> isn't supported by the hardware?". > >> > >> My stand, FWIW, is that igb and ixgbe should have the same > >> semantics as i40e currently has, i.e. return an error to the user > >> if the port is mis-configured, NOT changing the setting behind the > >> users back. > >> > > > > Fine by me, but then it means that the fix haw to include changes > > for all apps plus ixgbe and igb PMDs, correct? :) > > Ugh. Correct, I guess. :-) > > As for ixgbe and igb - they need a patch changing from silent ignore to > explicit error. Regarding the apps, I guess all the apps that rely on > that disabling CRC stripping always work, need some work. Or should all > the example applications have CRC stripping *enabled* by default? I'd > assume that all DPDK supported NICs has support for CRC stripping
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
2016-11-09 11:05, Bj?rn T?pel: > > BTW, all other examples would experience same problem too, right? > > Correct, so the broader question would be "what is the correct behavior > for an example application, when a port configuration isn't supported by > the hardware?". > > My stand, FWIW, is that igb and ixgbe should have the same semantics as > i40e currently has, i.e. return an error to the user if the port is > mis-configured, NOT changing the setting behind the users back. Yes it sounds sane.
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
> > Adding Helin to the conversation. > > > That's a common problem across Intel VF devices. Bothe igb and ixgbe > > overcomes that problem just by forcing ' rxmode.hw_strip_crc = 1;' at > > PMD itself: > > [...] > > > Wonder, can't i40e VF do the same? > > Right, however, and this (silent failure) approach was rejected by the > maintainers. Please, refer to this thread: > http://dpdk.org/ml/archives/dev/2016-April/037523.html > > > BTW, all other examples would experience same problem too, right? > > Correct, so the broader question would be "what is the correct behavior > for an example application, when a port configuration isn't supported by > the hardware?". > > My stand, FWIW, is that igb and ixgbe should have the same semantics as > i40e currently has, i.e. return an error to the user if the port is > mis-configured, NOT changing the setting behind the users back. > Fine by me, but then it means that the fix haw to include changes for all apps plus ixgbe and igb PMDs, correct? :) Konstantin
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Adding Helin to the conversation. > That's a common problem across Intel VF devices. Bothe igb and ixgbe > overcomes that problem just by forcing ' rxmode.hw_strip_crc = 1;' at > PMD itself: [...] > Wonder, can't i40e VF do the same? Right, however, and this (silent failure) approach was rejected by the maintainers. Please, refer to this thread: http://dpdk.org/ml/archives/dev/2016-April/037523.html > BTW, all other examples would experience same problem too, right? Correct, so the broader question would be "what is the correct behavior for an example application, when a port configuration isn't supported by the hardware?". My stand, FWIW, is that igb and ixgbe should have the same semantics as i40e currently has, i.e. return an error to the user if the port is mis-configured, NOT changing the setting behind the users back. Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
> Thanks for raising the issue. It is completely defeating the generic > ethdev API. We must not have different behaviours depending of the > driver. Why it cannot be fixed in the driver? I should probably refer to the thread, where the concern was raised: http://dpdk.org/ml/archives/dev/2016-July/044555.html So, the issue is that i40evf *only support* CRC stripping for some setups (i40e Linux driver for PF, i40evf DPDK driver VF). The l3fwd application disables CRC stripping for all ports, which leads to i40evf_dev_configure() failure -- which from my POV is correct. Mis-configuring a port shouldn't be allowed. I'm open to suggestions here. What would be a better way to solve this? Maybe just adding a command-line option to the l3fwd application is a better way around? Bj?rn
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
2016-11-09 09:23, Bj?rn T?pel:
> Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC
> stripping config") broke l3fwd, since it was forcing that CRC was
> kept. Now, if i40evf is running, CRC stripping will be enabled.
[...]
> + rte_eth_dev_info_get(portid, _info);
> + if (dev_info.driver_name &&
> + strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
> + /* i40evf require that CRC stripping is enabled. */
> + port_conf.rxmode.hw_strip_crc = 1;
> + } else {
> + port_conf.rxmode.hw_strip_crc = 0;
> + }
Thanks for raising the issue.
It is completely defeating the generic ethdev API.
We must not have different behaviours depending of the driver.
Why it cannot be fixed in the driver?
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Hi,
>
> Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC
> stripping config") broke l3fwd, since it was forcing that CRC was
> kept. Now, if i40evf is running, CRC stripping will be enabled.
>
> Signed-off-by: Bj?rn T?pel
> ---
> examples/l3fwd/main.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 7223e773107e..b60278794135 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -906,6 +906,14 @@ main(int argc, char **argv)
> n_tx_queue = MAX_TX_QUEUE_PER_PORT;
> printf("Creating queues: nb_rxq=%d nb_txq=%u... ",
> nb_rx_queue, (unsigned)n_tx_queue );
> + rte_eth_dev_info_get(portid, _info);
> + if (dev_info.driver_name &&
> + strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
> + /* i40evf require that CRC stripping is enabled. */
> + port_conf.rxmode.hw_strip_crc = 1;
> + } else {
> + port_conf.rxmode.hw_strip_crc = 0;
> + }
That's a common problem across Intel VF devices.
Bothe igb and ixgbe overcomes that problem just by forcing '
rxmode.hw_strip_crc = 1;'
at PMD itself:
static int
ixgbevf_dev_configure(struct rte_eth_dev *dev)
{
...
/*
* VF has no ability to enable/disable HW CRC
* Keep the persistent behavior the same as Host PF
*/
#ifndef RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC
if (!conf->rxmode.hw_strip_crc) {
PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
conf->rxmode.hw_strip_crc = 1;
}
#else
if (conf->rxmode.hw_strip_crc) {
PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
conf->rxmode.hw_strip_crc = 0;
}
#endif
Wonder, can't i40e VF do the same?
BTW, all other examples would experience same problem too, right?
Konstantin
> ret = rte_eth_dev_configure(portid, nb_rx_queue,
> (uint16_t)n_tx_queue, _conf);
> if (ret < 0)
> @@ -946,7 +954,6 @@ main(int argc, char **argv)
> printf("txq=%u,%d,%d ", lcore_id, queueid, socketid);
> fflush(stdout);
>
> - rte_eth_dev_info_get(portid, _info);
> txconf = _info.default_txconf;
> if (port_conf.rxmode.jumbo_frame)
> txconf->txq_flags = 0;
> --
> 2.9.3
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC
stripping config") broke l3fwd, since it was forcing that CRC was
kept. Now, if i40evf is running, CRC stripping will be enabled.
Signed-off-by: Bj?rn T?pel
---
examples/l3fwd/main.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 7223e773107e..b60278794135 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -906,6 +906,14 @@ main(int argc, char **argv)
n_tx_queue = MAX_TX_QUEUE_PER_PORT;
printf("Creating queues: nb_rxq=%d nb_txq=%u... ",
nb_rx_queue, (unsigned)n_tx_queue );
+ rte_eth_dev_info_get(portid, _info);
+ if (dev_info.driver_name &&
+ strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
+ /* i40evf require that CRC stripping is enabled. */
+ port_conf.rxmode.hw_strip_crc = 1;
+ } else {
+ port_conf.rxmode.hw_strip_crc = 0;
+ }
ret = rte_eth_dev_configure(portid, nb_rx_queue,
(uint16_t)n_tx_queue, _conf);
if (ret < 0)
@@ -946,7 +954,6 @@ main(int argc, char **argv)
printf("txq=%u,%d,%d ", lcore_id, queueid, socketid);
fflush(stdout);
- rte_eth_dev_info_get(portid, _info);
txconf = _info.default_txconf;
if (port_conf.rxmode.jumbo_frame)
txconf->txq_flags = 0;
--
2.9.3
[dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
Tested-by: Lei Yao
- Apply patch to v16.11-rc3
- Compile: Pass
- Host OS: VMware ESXi 6.0
- VM OS: Fedora 20
- GCC: 4.8.3
Tested with this patch, l3fwd sample can work with i40e VF with Fedora VM using
VMware as the host.
-Original Message-
From: Topel, Bjorn
Sent: Wednesday, November 9, 2016 4:24 PM
To: dev at dpdk.org
Cc: Xu, Qian Q ; Yao, Lei A ;
Wu, Jingjing ; thomas.monjalon at 6wind.com; Topel,
Bjorn
Subject: [PATCH] examples/l3fwd: force CRC stripping for i40evf
Commit 1bbcc5d21129 ("i40evf: report error for unsupported CRC stripping
config") broke l3fwd, since it was forcing that CRC was kept. Now, if i40evf is
running, CRC stripping will be enabled.
Signed-off-by: Bj?rn T?pel
---
examples/l3fwd/main.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c index
7223e773107e..b60278794135 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -906,6 +906,14 @@ main(int argc, char **argv)
n_tx_queue = MAX_TX_QUEUE_PER_PORT;
printf("Creating queues: nb_rxq=%d nb_txq=%u... ",
nb_rx_queue, (unsigned)n_tx_queue );
+ rte_eth_dev_info_get(portid, _info);
+ if (dev_info.driver_name &&
+ strcmp(dev_info.driver_name, "net_i40e_vf") == 0) {
+ /* i40evf require that CRC stripping is enabled. */
+ port_conf.rxmode.hw_strip_crc = 1;
+ } else {
+ port_conf.rxmode.hw_strip_crc = 0;
+ }
ret = rte_eth_dev_configure(portid, nb_rx_queue,
(uint16_t)n_tx_queue, _conf);
if (ret < 0)
@@ -946,7 +954,6 @@ main(int argc, char **argv)
printf("txq=%u,%d,%d ", lcore_id, queueid, socketid);
fflush(stdout);
- rte_eth_dev_info_get(portid, _info);
txconf = _info.default_txconf;
if (port_conf.rxmode.jumbo_frame)
txconf->txq_flags = 0;
--
2.9.3
