[dpdk-dev] DPDK Summit Userspace - streaming live
On Thu, 2016-10-20 at 11:04 +, St Leger, Jim wrote: > If you couldn't make it to the Userspace event we are streaming it > live on Periscope.? https://www.periscope.tv/jimstleger > Thank you. This is much appreciated. Regards, Frederico
[dpdk-dev] [PATCH] testpmd: fix fdir command on MAC and tunnel modes
On Tue, Sep 27, 2016 at 4:42 AM, Wu, Jingjing wrote:
>
>
>> -Original Message-
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Frederico.Cadete-
>> ext at oneaccess-net.com
>> Sent: Tuesday, August 23, 2016 5:11 PM
>> To: dev at dpdk.org
>> Cc: frederico at cadete.eu; Frederico Cadete
>> Subject: [dpdk-dev] [PATCH] testpmd: fix fdir command on MAC and tunnel
>> modes
>>
>> From: Frederico Cadete
>>
>> The flow_director_filter commands has a pf|vf option for most modes
>> except for MAC-VLAN and tunnel. On Intel NIC's these modes are not
>> supported under virtualized environments.
>> But the application was checking that this field was parsed for these cases,
>> even though this token is not registered with the cmdline parser.
>>
>> This patch skips checking of this field for the commands that don't accept
>> it.
>>
>> Signed-off-by: Frederico Cadete
>> ---
>> app/test-pmd/cmdline.c | 32 ++--
>> 1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>> f90befc..f516b1b 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -8502,24 +8502,28 @@ cmd_flow_director_filter_parsed(void
>> *parsed_result,
>> else
>> entry.action.behavior = RTE_ETH_FDIR_ACCEPT;
>>
>> - if (!strcmp(res->pf_vf, "pf"))
>> - entry.input.flow_ext.is_vf = 0;
>> - else if (!strncmp(res->pf_vf, "vf", 2)) {
>> - struct rte_eth_dev_info dev_info;
>> + if (fdir_conf.mode != RTE_FDIR_MODE_PERFECT_MAC_VLAN &&
>> + fdir_conf.mode != RTE_FDIR_MODE_PERFECT_TUNNEL){
>>
>> - memset(&dev_info, 0, sizeof(dev_info));
>> - rte_eth_dev_info_get(res->port_id, &dev_info);
>> - errno = 0;
>> - vf_id = strtoul(res->pf_vf + 2, &end, 10);
>> - if (errno != 0 || *end != '\0' || vf_id >= dev_info.max_vfs) {
>> + if (!strcmp(res->pf_vf, "pf"))
>> + entry.input.flow_ext.is_vf = 0;
>> + else if (!strncmp(res->pf_vf, "vf", 2)) {
>> + struct rte_eth_dev_info dev_info;
>> +
>> + memset(&dev_info, 0, sizeof(dev_info));
>> + rte_eth_dev_info_get(res->port_id, &dev_info);
>> + errno = 0;
>> + vf_id = strtoul(res->pf_vf + 2, &end, 10);
>> + if (errno != 0 || *end != '\0' || vf_id >=
>> dev_info.max_vfs) {
>> + printf("invalid parameter %s.\n", res->pf_vf);
>> + return;
>> + }
>> + entry.input.flow_ext.is_vf = 1;
>> + entry.input.flow_ext.dst_id = (uint16_t)vf_id;
>> + } else {
>> printf("invalid parameter %s.\n", res->pf_vf);
>> return;
>> }
>> - entry.input.flow_ext.is_vf = 1;
>> - entry.input.flow_ext.dst_id = (uint16_t)vf_id;
>> - } else {
>> - printf("invalid parameter %s.\n", res->pf_vf);
>> - return;
>> }
>
> Thanks for the patch.
And thanks a lot for the review.
> But with this change the field of pf_vf cannot omit either.
> I think it still looks confused because it will allow any meaningless string.
Sorry, I am not aware that it can be omitted.
For MAC/VLAN and tunnel mode it does not and will not allow any
meaningless string.
At least that was my intention :)
The cmdline parser expects "... flexbytes (flexbytes_value) (drop|fwd)
queue ..." .
This is what is documented [1] and the command's cmdline_parse_inst_t
[2] matches this.
If you put something in-between "(drop|fwd)" and "queue" it is
rejected by the parser
in librte_cmdline.
> In MAC_VLAN or TUNNEL mode, why not just use pf.
With the current code, because if you write that in the command, it is
rejected by the parser :)
Do you mean it would be preferable to make these commands always take
such an argument,
and only at the NIC driver check that it must equal PF for MAC_VLAN or
TUNNEL mode?
The command becomes a bit more complicated for the current intel
NIC's, but as I understand
it currently does not work anyway. Unless I'm missing something else.
>
> Maybe an optional field supporting on DPDK cmdline library is exactly what we
> Are waiting for :)
Laudable goal! My excuses but it's beyond my current skills and bandwith :/
Regards,
Frederico
[1] http://dpdk.org/browse/dpdk/tree/app/test-pmd/cmdline.c#n703
[2] http://dpdk.org/browse/dpdk/tree/app/test-pmd/cmdline.c#n8821
>
>
> Thanks
> Jingjing
[dpdk-dev] bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel
Hello, I believe commit 260aae9a [1] has introduced a regression for the case of 32-bit process running on a 64-bit kernel. The commit is effectively casting mbuf->buf_physaddr to uintptr_t before dereferencing it. It truncates the physical address to the width of the process's uint, and in the the aforementioned combination this loses important bits. I can confirm this under GDB. When virtqueue_enqueue_xmit is filling in start_dp, I get this result: (gdb) p /x cookie->buf_physaddr $5 = 0x12f94a000 (gdb) p /x start_dp[idx].addr $6 = 0x2f94a080 On my system, I capture the packet that goes out to the host and I see it has the correct size but the content is all-zeroes. I would like to propose a patch that would work for all supported combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. But I don't really see how that could work, given virtio-user tries to store a physical address in rte_mbuf's "void *buf_addr", and this is not always big enough for a physical address. Any suggestions on if and how this could be fixed? Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users not requiring virtio-user support can avoid it by setting CONFIG_VIRTIO_USER=n during compilation. Best regards, Frederico Cadete
Re: [dpdk-dev] bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel
On Fri, 2017-06-23 at 23:36 +0800, Tan, Jianfeng wrote: > Hi Cadete, > > > On 6/22/2017 10:58 PM, Frederico Cadete wrote: > > > > Hello, > > > > I believe commit 260aae9a [1] has introduced a regression for the > > case > > of 32-bit process running on a 64-bit kernel. > > > > The commit is effectively casting mbuf->buf_physaddr to uintptr_t > > before dereferencing it. It truncates the physical address to the > > width > > of the process's uint, and in the the aforementioned combination > > this > > loses important bits. > > > > I can confirm this under GDB. When virtqueue_enqueue_xmit is > > filling in > > start_dp, I get this result: > > > > (gdb) p /x cookie->buf_physaddr > > $5 = 0x12f94a000 > > (gdb) p /x start_dp[idx].addr > > $6 = 0x2f94a080 > Now you are testing a virtio-pci device and app is compiled into a > 32-bit executable on 64-bit VM system? Exactly. Furthermore, this bug is only visible if you give the virtual machine enough memory for the mbuf's physiscal address to be above the 4GB mark. > > > > > > > On my system, I capture the packet that goes out to the host and I > > see > > it has the correct size but the content is all-zeroes. > > > > I would like to propose a patch that would work for all supported > > combinations of user/kernel bitwidth *and* virtio-pci/virtio-user. > > But > > I don't really see how that could work, given virtio-user tries to > > store a physical address in rte_mbuf's "void *buf_addr", and this > > is > > not always big enough for a physical address. > virtio-user does not store a physical address in rte_mbuf's "void > *buf_addr", instead, it uses this field in rte_mbuf to fill desc's > addr > which is always 64bit long. Oh, that's right. Sorry about that. In that case I guess that the issue is that the conversion is assuming that on 32-bit apps only 4 bytes are necessary, even in the case of virtio-pci and 64-bit physaddr. Would you say that this is how vring_desc's addr should be filled? | 32-bit app | 64-bit app | +---+ ---+ virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes | virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes | I believe that the issue is that after commit 260aae9a, for virtio-pci and 32-bit app it is taking 4 bytes instead of 8. > > > > > Any suggestions on if and how this could be fixed? > > > > Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master. Users > > not > > requiring virtio-user support can avoid it by setting > > CONFIG_VIRTIO_USER=n during compilation. > > > > Best regards, > > Frederico Cadete
Re: [dpdk-dev] bug: virtio PMD sends malformed packets for 32-bit processes on 64-bit kernel
On Mon, 2017-06-26 at 18:15 +0800, Tan, Jianfeng wrote:
> On 6/26/2017 4:14 PM, Frederico Cadete wrote:
> >
> > On Fri, 2017-06-23 at 23:36 +0800, Tan, Jianfeng wrote:
> > >
> > > Hi Cadete,
> > >
> > >
> > > On 6/22/2017 10:58 PM, Frederico Cadete wrote:
> > > >
> > > > Hello,
> > > >
> > > > I believe commit 260aae9a [1] has introduced a regression for
> > > > the
> > > > case
> > > > of 32-bit process running on a 64-bit kernel.
> > > >
> > > > The commit is effectively casting mbuf->buf_physaddr to
> > > > uintptr_t
> > > > before dereferencing it. It truncates the physical address to
> > > > the
> > > > width
> > > > of the process's uint, and in the the aforementioned
> > > > combination
> > > > this
> > > > loses important bits.
> > > >
> > > > I can confirm this under GDB. When virtqueue_enqueue_xmit is
> > > > filling in
> > > > start_dp, I get this result:
> > > >
> > > > (gdb) p /x cookie->buf_physaddr
> > > > $5 = 0x12f94a000
> > > > (gdb) p /x start_dp[idx].addr
> > > > $6 = 0x2f94a080
> > > Now you are testing a virtio-pci device and app is compiled into
> > > a
> > > 32-bit executable on 64-bit VM system?
> > Exactly. Furthermore, this bug is only visible if you give the
> > virtual
> > machine enough memory for the mbuf's physiscal address to be above
> > the
> > 4GB mark.
> That's an important information.
>
> >
> >
> > >
> > > >
> > > >
> > > > On my system, I capture the packet that goes out to the host
> > > > and I
> > > > see
> > > > it has the correct size but the content is all-zeroes.
> > > >
> > > > I would like to propose a patch that would work for all
> > > > supported
> > > > combinations of user/kernel bitwidth *and* virtio-pci/virtio-
> > > > user.
> > > > But
> > > > I don't really see how that could work, given virtio-user tries
> > > > to
> > > > store a physical address in rte_mbuf's "void *buf_addr", and
> > > > this
> > > > is
> > > > not always big enough for a physical address.
> > > virtio-user does not store a physical address in rte_mbuf's "void
> > > *buf_addr", instead, it uses this field in rte_mbuf to fill
> > > desc's
> > > addr
> > > which is always 64bit long.
> > Oh, that's right. Sorry about that.
> >
> > In that case I guess that the issue is that the conversion is
> > assuming
> > that on 32-bit apps only 4 bytes are necessary, even in the case of
> > virtio-pci and 64-bit physaddr.
> >
> > Would you say that this is how vring_desc's addr should be filled?
> >
> > | 32-bit app | 64-bit app |
> > +---+ ---+
> > virtio-pci | buf_physaddr, 8 bytes | buf_physaddr, 8 bytes |
> > virtio-user | buf_addr, 4 bytes | buf_addr, 8 bytes |
> >
> > I believe that the issue is that after commit 260aae9a, for virtio-
> > pci
> > and 32-bit app it is taking 4 bytes instead of 8.
> Aha, yes, that's the issue! Great analysis. After Bruce's commit
> 586ec205bcbbb ("mbuf: fix 64-bit address alignment in 32-bit
> builds"),
> we can fix this issue by fetching 8 bytes at all cases. But
> unfortunately, that commit is not back-ported to v17.02.1.
I don't see how changing the alignment of buf_physaddr allows fetching
8 bytes in all cases, even in the case of 32-bit virtio-user where what
we need are 4 bytes from buf_addr. Am I missing something?
Besides, Bruce's patch changes the memory layout of rte_mbuf. A priori
that's not the kind I would like to find in an update of a stable
branch :)
>
> I wonder if we can back-port Bruce's patch with a new patch to fix
> this
> problem?
>
> Any opinions from others?
>
> Thanks,
> Jianfeng
>
> >
> >
> > >
> > > >
> > > > Any suggestions on if and how this could be fixed?
> > > >
> > > > Meanwhile, the bug affects dpdk 17.05, 17.02.1 and master.
> > > > Users
> > > > not
> > > > requiring virtio-user support can avoid it by setting
> > > > CONFIG_VIRTIO_USER=n during compilation.
> > > >
> > > > Best regards,
> > > > Frederico Cadete
