DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
@ 2016-11-10 13:50 Mori, Naoyuki
  2016-11-10 14:32 ` Thomas Monjalon
  0 siblings, 1 reply; 19+ messages in thread
From: Mori, Naoyuki @ 2016-11-10 13:50 UTC (permalink / raw)
  To: dev, bjorn.topel, Yao, Lei A, Topel, Bjorn, Zhang, Helin,
	Ananyev, Konstantin
  Cc: Xu, Qian Q, Wu, Jingjing, thomas.monjalon

Hi,

Re:
>    Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping
>    	for	i40evf
>    Message-ID: <cce4d31c-8a84-07aa-3361-327dba1ec77a@intel.com>
>    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)
{
<snip>
        /* 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;
                }
        }
<snip>
}
---

ixgbevf is same Intel NIC but implementation is different. It won’t return error.

ixgbevf_dev_configure(struct rte_eth_dev *dev)
{
<snip>
        /*
         * 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
<snip>
}

Regards and Thanks,
Mori


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-10 13:50 [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf Mori, Naoyuki
@ 2016-11-10 14:32 ` Thomas Monjalon
  2016-11-10 14:43   ` Mori, Naoyuki
  0 siblings, 1 reply; 19+ messages in thread
From: Thomas Monjalon @ 2016-11-10 14:32 UTC (permalink / raw)
  To: Mori, Naoyuki
  Cc: dev, bjorn.topel, Yao, Lei A, Topel, Bjorn, Zhang, Helin,
	Ananyev, Konstantin, Xu, Qian Q, Wu, Jingjing

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-10 14:32 ` Thomas Monjalon
@ 2016-11-10 14:43   ` Mori, Naoyuki
  0 siblings, 0 replies; 19+ messages in thread
From: Mori, Naoyuki @ 2016-11-10 14:43 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev, bjorn.topel, Yao, Lei A, Topel, Bjorn, Zhang, Helin,
	Ananyev, Konstantin, Xu, Qian Q, Wu, Jingjing

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-10  7:55                 ` Thomas Monjalon
@ 2016-11-10  7:59                   ` Björn Töpel
  0 siblings, 0 replies; 19+ messages in thread
From: Björn Töpel @ 2016-11-10  7:59 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Yao, Lei A, Zhang, Helin, Ananyev, Konstantin, dev, Xu, Qian Q,
	Wu, Jingjing

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-10  6:17               ` Björn Töpel
@ 2016-11-10  7:55                 ` Thomas Monjalon
  2016-11-10  7:59                   ` Björn Töpel
  0 siblings, 1 reply; 19+ messages in thread
From: Thomas Monjalon @ 2016-11-10  7:55 UTC (permalink / raw)
  To: Björn Töpel, Topel, Bjorn
  Cc: Yao, Lei A, Zhang, Helin, Ananyev, Konstantin, dev, Xu, Qian Q,
	Wu, Jingjing

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?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-10  5:49             ` Yao, Lei A
@ 2016-11-10  6:17               ` Björn Töpel
  2016-11-10  7:55                 ` Thomas Monjalon
  0 siblings, 1 reply; 19+ messages in thread
From: Björn Töpel @ 2016-11-10  6:17 UTC (permalink / raw)
  To: Yao, Lei A, Topel, Bjorn, Zhang, Helin, Ananyev, Konstantin, dev
  Cc: Xu, Qian Q, Wu, Jingjing, thomas.monjalon

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 13:09           ` Björn Töpel
@ 2016-11-10  5:49             ` Yao, Lei A
  2016-11-10  6:17               ` Björn Töpel
  0 siblings, 1 reply; 19+ messages in thread
From: Yao, Lei A @ 2016-11-10  5:49 UTC (permalink / raw)
  To: Topel, Bjorn, Zhang, Helin, Ananyev, Konstantin, dev
  Cc: Xu, Qian Q, Wu, Jingjing, thomas.monjalon

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 <helin.zhang@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
Cc: Xu, Qian Q <qian.q.xu@intel.com>; Yao, Lei A <lei.a.yao@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; thomas.monjalon@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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 13:01         ` Zhang, Helin
@ 2016-11-09 13:09           ` Björn Töpel
  2016-11-10  5:49             ` Yao, Lei A
  0 siblings, 1 reply; 19+ messages in thread
From: Björn Töpel @ 2016-11-09 13:09 UTC (permalink / raw)
  To: Zhang, Helin, Ananyev, Konstantin, dev
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 11:27       ` Björn Töpel
  2016-11-09 12:13         ` Ananyev, Konstantin
@ 2016-11-09 13:01         ` Zhang, Helin
  2016-11-09 13:09           ` Björn Töpel
  1 sibling, 1 reply; 19+ messages in thread
From: Zhang, Helin @ 2016-11-09 13:01 UTC (permalink / raw)
  To: Topel, Bjorn, Ananyev, Konstantin, dev
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon



> -----Original Message-----
> From: Topel, Bjorn
> Sent: Wednesday, November 9, 2016 7:28 PM
> To: Ananyev, Konstantin; dev@dpdk.org; Zhang, Helin
> Cc: Xu, Qian Q; Yao, Lei A; Wu, Jingjing; thomas.monjalon@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 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

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.
Any critical issue now? Or just an improvement comments?

Thanks,
Helin


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 11:27       ` Björn Töpel
@ 2016-11-09 12:13         ` Ananyev, Konstantin
  2016-11-09 13:01         ` Zhang, Helin
  1 sibling, 0 replies; 19+ messages in thread
From: Ananyev, Konstantin @ 2016-11-09 12:13 UTC (permalink / raw)
  To: Topel, Bjorn, dev, Zhang, Helin
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon



> -----Original Message-----
> From: Topel, Bjorn
> Sent: Wednesday, November 9, 2016 11:28 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>
> Cc: Xu, Qian Q <qian.q.xu@intel.com>; Yao, Lei A <lei.a.yao@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> thomas.monjalon@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

From the sources, it seems that only nfp doesn't support HW CRC stripping.
In fact, as I can see some non-Intel PMDs just ignore hw_stip_crc value
and assume that CRC strip is always on. 

> 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?

I don’t know any good reason for that for l3fwd or any other sample app.
I think it is just a 'historical' reason.
Konstantin

> 
> 
> Björn


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 11:08     ` Ananyev, Konstantin
@ 2016-11-09 11:27       ` Björn Töpel
  2016-11-09 12:13         ` Ananyev, Konstantin
  2016-11-09 13:01         ` Zhang, Helin
  0 siblings, 2 replies; 19+ messages in thread
From: Björn Töpel @ 2016-11-09 11:27 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev, Zhang, Helin
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 10:05   ` Björn Töpel
  2016-11-09 10:22     ` Thomas Monjalon
@ 2016-11-09 11:08     ` Ananyev, Konstantin
  2016-11-09 11:27       ` Björn Töpel
  1 sibling, 1 reply; 19+ messages in thread
From: Ananyev, Konstantin @ 2016-11-09 11:08 UTC (permalink / raw)
  To: Topel, Bjorn, dev, Zhang, Helin
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09 10:05   ` Björn Töpel
@ 2016-11-09 10:22     ` Thomas Monjalon
  2016-11-09 11:08     ` Ananyev, Konstantin
  1 sibling, 0 replies; 19+ messages in thread
From: Thomas Monjalon @ 2016-11-09 10:22 UTC (permalink / raw)
  To: Björn Töpel
  Cc: Ananyev, Konstantin, dev, helin.zhang, Xu, Qian Q, Yao, Lei A,
	Wu, Jingjing

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09  9:46 ` Ananyev, Konstantin
@ 2016-11-09 10:05   ` Björn Töpel
  2016-11-09 10:22     ` Thomas Monjalon
  2016-11-09 11:08     ` Ananyev, Konstantin
  0 siblings, 2 replies; 19+ messages in thread
From: Björn Töpel @ 2016-11-09 10:05 UTC (permalink / raw)
  To: Ananyev, Konstantin, dev, helin.zhang
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09  8:23 Björn Töpel
  2016-11-09  8:37 ` Yao, Lei A
  2016-11-09  9:28 ` Thomas Monjalon
@ 2016-11-09  9:46 ` Ananyev, Konstantin
  2016-11-09 10:05   ` Björn Töpel
  2 siblings, 1 reply; 19+ messages in thread
From: Ananyev, Konstantin @ 2016-11-09  9:46 UTC (permalink / raw)
  To: Topel, Bjorn, dev
  Cc: Xu, Qian Q, Yao, Lei A, Wu, Jingjing, thomas.monjalon, Topel, Bjorn


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 <bjorn.topel@intel.com>
> ---
>  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, &dev_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, &port_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, &dev_info);
>  			txconf = &dev_info.default_txconf;
>  			if (port_conf.rxmode.jumbo_frame)
>  				txconf->txq_flags = 0;
> --
> 2.9.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09  9:28 ` Thomas Monjalon
@ 2016-11-09  9:39   ` Björn Töpel
  0 siblings, 0 replies; 19+ messages in thread
From: Björn Töpel @ 2016-11-09  9:39 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, qian.q.xu, lei.a.yao, jingjing.wu

> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09  8:23 Björn Töpel
  2016-11-09  8:37 ` Yao, Lei A
@ 2016-11-09  9:28 ` Thomas Monjalon
  2016-11-09  9:39   ` Björn Töpel
  2016-11-09  9:46 ` Ananyev, Konstantin
  2 siblings, 1 reply; 19+ messages in thread
From: Thomas Monjalon @ 2016-11-09  9:28 UTC (permalink / raw)
  To: Björn Töpel; +Cc: dev, qian.q.xu, lei.a.yao, jingjing.wu

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, &dev_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?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
  2016-11-09  8:23 Björn Töpel
@ 2016-11-09  8:37 ` Yao, Lei A
  2016-11-09  9:28 ` Thomas Monjalon
  2016-11-09  9:46 ` Ananyev, Konstantin
  2 siblings, 0 replies; 19+ messages in thread
From: Yao, Lei A @ 2016-11-09  8:37 UTC (permalink / raw)
  To: Topel, Bjorn, dev; +Cc: Xu, Qian Q, Wu, Jingjing, thomas.monjalon

Tested-by: Lei Yao <lei.a.yao@intel.com>
- 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@dpdk.org
Cc: Xu, Qian Q <qian.q.xu@intel.com>; Yao, Lei A <lei.a.yao@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; thomas.monjalon@6wind.com; Topel, Bjorn <bjorn.topel@intel.com>
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 <bjorn.topel@intel.com>
---
 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, &dev_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, &port_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, &dev_info);
 			txconf = &dev_info.default_txconf;
 			if (port_conf.rxmode.jumbo_frame)
 				txconf->txq_flags = 0;
--
2.9.3


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf
@ 2016-11-09  8:23 Björn Töpel
  2016-11-09  8:37 ` Yao, Lei A
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Björn Töpel @ 2016-11-09  8:23 UTC (permalink / raw)
  To: dev
  Cc: qian.q.xu, lei.a.yao, jingjing.wu, thomas.monjalon,
	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.

Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
 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, &dev_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, &port_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, &dev_info);
 			txconf = &dev_info.default_txconf;
 			if (port_conf.rxmode.jumbo_frame)
 				txconf->txq_flags = 0;
-- 
2.9.3

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2016-11-10 14:43 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-10 13:50 [dpdk-dev] [PATCH] examples/l3fwd: force CRC stripping for i40evf Mori, Naoyuki
2016-11-10 14:32 ` Thomas Monjalon
2016-11-10 14:43   ` Mori, Naoyuki
  -- strict thread matches above, loose matches on Subject: below --
2016-11-09  8:23 Björn Töpel
2016-11-09  8:37 ` Yao, Lei A
2016-11-09  9:28 ` Thomas Monjalon
2016-11-09  9:39   ` Björn Töpel
2016-11-09  9:46 ` Ananyev, Konstantin
2016-11-09 10:05   ` Björn Töpel
2016-11-09 10:22     ` Thomas Monjalon
2016-11-09 11:08     ` Ananyev, Konstantin
2016-11-09 11:27       ` Björn Töpel
2016-11-09 12:13         ` Ananyev, Konstantin
2016-11-09 13:01         ` Zhang, Helin
2016-11-09 13:09           ` Björn Töpel
2016-11-10  5:49             ` Yao, Lei A
2016-11-10  6:17               ` Björn Töpel
2016-11-10  7:55                 ` Thomas Monjalon
2016-11-10  7:59                   ` Björn Töpel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).