app/testpmd: fix MPLSoUDP encapsulation
Checks
Commit Message
Set MPLS label value in appropriate location at mplsoudp_encap_conf,
so it is correctly copied to rte_flow_item_mpls.
Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
Cc: orika@mellanox.com
Signed-off-by: Dekel Peled <dekelp@mellanox.com>
---
app/test-pmd/cmdline.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> Sent: Monday, November 19, 2018 6:55 PM
> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> Subject: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>
> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> so it is correctly copied to rte_flow_item_mpls.
>
> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> Cc: orika@mellanox.com
>
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
> app/test-pmd/cmdline.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 1275074..40e64cc 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15804,10 +15804,10 @@ static void
> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> union {
> uint32_t mplsoudp_label;
> - uint8_t label[3];
> + uint8_t label[4];
> } id = {
> .mplsoudp_label =
> - rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> + rte_cpu_to_be_32(res->label<<4) &
> RTE_BE32(0x00ffffff),
> };
>
> if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> --
> 1.8.3.1
Acked-by: Ori Kam <orika@mellanox.com>
Thanks,
Ori
On 11/20/2018 8:23 AM, Ori Kam wrote:
>
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
>> Sent: Monday, November 19, 2018 6:55 PM
>> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
>> bernard.iremonger@intel.com
>> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
>> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>>
>> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
>> so it is correctly copied to rte_flow_item_mpls.
>>
>> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
>> Cc: orika@mellanox.com
>>
>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
>> ---
>> app/test-pmd/cmdline.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 1275074..40e64cc 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -15804,10 +15804,10 @@ static void
>> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>> struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>> union {
>> uint32_t mplsoudp_label;
>> - uint8_t label[3];
>> + uint8_t label[4];
>> } id = {
>> .mplsoudp_label =
>> - rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
>> + rte_cpu_to_be_32(res->label<<4) &
>> RTE_BE32(0x00ffffff),
>> };
>>
>> if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
>> --
>> 1.8.3.1
>
> Acked-by: Ori Kam <orika@mellanox.com>
Hi Ori, Dekel,
What is the scope of this patch? Briefly how critical it is and what will be
broken and what is exposure of it?
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, November 21, 2018 5:19 PM
> To: Ori Kam <orika@mellanox.com>; Dekel Peled <dekelp@mellanox.com>;
> wenzhuo.lu@intel.com; jingjing.wu@intel.com; bernard.iremonger@intel.com
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>
> On 11/20/2018 8:23 AM, Ori Kam wrote:
> >
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Dekel Peled
> >> Sent: Monday, November 19, 2018 6:55 PM
> >> To: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> >> bernard.iremonger@intel.com
> >> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> >> <shahafs@mellanox.com>; Dekel Peled <dekelp@mellanox.com>
> >> Subject: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> >>
> >> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> >> so it is correctly copied to rte_flow_item_mpls.
> >>
> >> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> >> Cc: orika@mellanox.com
> >>
> >> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> >> ---
> >> app/test-pmd/cmdline.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> >> index 1275074..40e64cc 100644
> >> --- a/app/test-pmd/cmdline.c
> >> +++ b/app/test-pmd/cmdline.c
> >> @@ -15804,10 +15804,10 @@ static void
> >> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> >> struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> >> union {
> >> uint32_t mplsoudp_label;
> >> - uint8_t label[3];
> >> + uint8_t label[4];
> >> } id = {
> >> .mplsoudp_label =
> >> - rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> >> + rte_cpu_to_be_32(res->label<<4) &
> >> RTE_BE32(0x00ffffff),
> >> };
> >>
> >> if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> >> --
> >> 1.8.3.1
> >
> > Acked-by: Ori Kam <orika@mellanox.com>
>
> Hi Ori, Dekel,
>
> What is the scope of this patch? Briefly how critical it is and what will be
> broken and what is exposure of it?
The only issue is that we are setting incorrect MPLS label.
As defined by the MPLS spec the label is 20 bits, so this patch simply
pushes the label to the correct place.
I don't think that there any exposure from this patch.
Best,
Ori
On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> so it is correctly copied to rte_flow_item_mpls.
>
> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> Cc: orika@mellanox.com
>
> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> ---
> app/test-pmd/cmdline.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 1275074..40e64cc 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -15804,10 +15804,10 @@ static void cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> union {
> uint32_t mplsoudp_label;
> - uint8_t label[3];
> + uint8_t label[4];
> } id = {
> .mplsoudp_label =
> - rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
> + rte_cpu_to_be_32(res->label<<4) & RTE_BE32(0x00ffffff),
Just to be sure, since label is a 20-bit value, isn't the shift supposed to
be 12 bits? In which case that mask is harmless but misleading. How about:
.mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
> };
>
> if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> --
> 1.8.3.1
>
Thanks, PSB.
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, November 22, 2018 11:05 AM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>
> On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> > Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> > so it is correctly copied to rte_flow_item_mpls.
> >
> > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > Cc: orika@mellanox.com
> >
> > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > ---
> > app/test-pmd/cmdline.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 1275074..40e64cc 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -15804,10 +15804,10 @@ static void
> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> > struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> > union {
> > uint32_t mplsoudp_label;
> > - uint8_t label[3];
> > + uint8_t label[4];
> > } id = {
> > .mplsoudp_label =
> > - rte_cpu_to_be_32(res->label) &
> RTE_BE32(0x00ffffff),
> > + rte_cpu_to_be_32(res->label<<4) &
> RTE_BE32(0x00ffffff),
>
> Just to be sure, since label is a 20-bit value, isn't the shift supposed to be 12
> bits? In which case that mask is harmless but misleading. How about:
>
> .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
>
Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.
> > };
> >
> > if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)
> > --
> > 1.8.3.1
> >
>
> --
> Adrien Mazarguil
> 6WIND
On Thu, Nov 22, 2018 at 09:56:09AM +0000, Dekel Peled wrote:
> Thanks, PSB.
>
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Thursday, November 22, 2018 11:05 AM
> > To: Dekel Peled <dekelp@mellanox.com>
> > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> > <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
> >
> > On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> > > Set MPLS label value in appropriate location at mplsoudp_encap_conf,
> > > so it is correctly copied to rte_flow_item_mpls.
> > >
> > > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > > Cc: orika@mellanox.com
> > >
> > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > ---
> > > app/test-pmd/cmdline.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > 1275074..40e64cc 100644
> > > --- a/app/test-pmd/cmdline.c
> > > +++ b/app/test-pmd/cmdline.c
> > > @@ -15804,10 +15804,10 @@ static void
> > cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> > > struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> > > union {
> > > uint32_t mplsoudp_label;
> > > - uint8_t label[3];
> > > + uint8_t label[4];
> > > } id = {
> > > .mplsoudp_label =
> > > - rte_cpu_to_be_32(res->label) &
> > RTE_BE32(0x00ffffff),
> > > + rte_cpu_to_be_32(res->label<<4) &
> > RTE_BE32(0x00ffffff),
> >
> > Just to be sure, since label is a 20-bit value, isn't the shift supposed to be 12
> > bits? In which case that mask is harmless but misleading. How about:
> >
> > .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
> >
>
> Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.
OK, I know, what I missed was the following line:
rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
Just a suggestion then: using the same memcpy() offsets in both places for
clarity:
rte_be32_t label = rte_cpu_to_be32(res->label << 12);
memcpy(mplsodudp_encap_conf.label, &label, 3);
Hi,
The current implementation is already validated, and since this is the last minute I prefer my patch to be applied as-is.
Please ack.
Regards,
Dekel
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Thursday, November 22, 2018 12:14 PM
> To: Dekel Peled <dekelp@mellanox.com>
> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>
> On Thu, Nov 22, 2018 at 09:56:09AM +0000, Dekel Peled wrote:
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Thursday, November 22, 2018 11:05 AM
> > > To: Dekel Peled <dekelp@mellanox.com>
> > > Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
> > > bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
> > > <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> > > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP
> > > encapsulation
> > >
> > > On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
> > > > Set MPLS label value in appropriate location at
> > > > mplsoudp_encap_conf, so it is correctly copied to rte_flow_item_mpls.
> > > >
> > > > Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
> > > > Cc: orika@mellanox.com
> > > >
> > > > Signed-off-by: Dekel Peled <dekelp@mellanox.com>
> > > > ---
> > > > app/test-pmd/cmdline.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > 1275074..40e64cc 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -15804,10 +15804,10 @@ static void
> > > cmd_set_mplsoudp_encap_parsed(void *parsed_result,
> > > > struct cmd_set_mplsoudp_encap_result *res = parsed_result;
> > > > union {
> > > > uint32_t mplsoudp_label;
> > > > - uint8_t label[3];
> > > > + uint8_t label[4];
> > > > } id = {
> > > > .mplsoudp_label =
> > > > - rte_cpu_to_be_32(res->label) &
> > > RTE_BE32(0x00ffffff),
> > > > + rte_cpu_to_be_32(res->label<<4) &
> > > RTE_BE32(0x00ffffff),
> > >
> > > Just to be sure, since label is a 20-bit value, isn't the shift
> > > supposed to be 12 bits? In which case that mask is harmless but
> misleading. How about:
> > >
> > > .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
> > >
> >
> > Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.
>
> OK, I know, what I missed was the following line:
>
> rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
>
> Just a suggestion then: using the same memcpy() offsets in both places for
> clarity:
>
> rte_be32_t label = rte_cpu_to_be32(res->label << 12);
>
> memcpy(mplsodudp_encap_conf.label, &label, 3);
>
> --
> Adrien Mazarguil
> 6WIND
On 11/22/2018 4:18 PM, Dekel Peled wrote:
> Hi,
>
> The current implementation is already validated, and since this is the last minute I prefer my patch to be applied as-is.
> Please ack.
Hi Dekel,
I think logic is other-way around, a patch has been acked clearly, without
question can justify to go in last minute. Going last minute doesn't justify an ack.
>
> Regards,
> Dekel
>
>> -----Original Message-----
>> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>> Sent: Thursday, November 22, 2018 12:14 PM
>> To: Dekel Peled <dekelp@mellanox.com>
>> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
>> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
>> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP encapsulation
>>
>> On Thu, Nov 22, 2018 at 09:56:09AM +0000, Dekel Peled wrote:
>>> Thanks, PSB.
>>>
>>>> -----Original Message-----
>>>> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>>> Sent: Thursday, November 22, 2018 11:05 AM
>>>> To: Dekel Peled <dekelp@mellanox.com>
>>>> Cc: wenzhuo.lu@intel.com; jingjing.wu@intel.com;
>>>> bernard.iremonger@intel.com; dev@dpdk.org; Ori Kam
>>>> <orika@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
>>>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix MPLSoUDP
>>>> encapsulation
>>>>
>>>> On Mon, Nov 19, 2018 at 06:54:50PM +0200, Dekel Peled wrote:
>>>>> Set MPLS label value in appropriate location at
>>>>> mplsoudp_encap_conf, so it is correctly copied to rte_flow_item_mpls.
>>>>>
>>>>> Fixes: a1191d39cb57 ("app/testpmd: add MPLSoUDP encapsulation")
>>>>> Cc: orika@mellanox.com
>>>>>
>>>>> Signed-off-by: Dekel Peled <dekelp@mellanox.com>
>>>>> ---
>>>>> app/test-pmd/cmdline.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>>>> 1275074..40e64cc 100644
>>>>> --- a/app/test-pmd/cmdline.c
>>>>> +++ b/app/test-pmd/cmdline.c
>>>>> @@ -15804,10 +15804,10 @@ static void
>>>> cmd_set_mplsoudp_encap_parsed(void *parsed_result,
>>>>> struct cmd_set_mplsoudp_encap_result *res = parsed_result;
>>>>> union {
>>>>> uint32_t mplsoudp_label;
>>>>> - uint8_t label[3];
>>>>> + uint8_t label[4];
>>>>> } id = {
>>>>> .mplsoudp_label =
>>>>> - rte_cpu_to_be_32(res->label) &
>>>> RTE_BE32(0x00ffffff),
>>>>> + rte_cpu_to_be_32(res->label<<4) &
>>>> RTE_BE32(0x00ffffff),
>>>>
>>>> Just to be sure, since label is a 20-bit value, isn't the shift
>>>> supposed to be 12 bits? In which case that mask is harmless but
>> misleading. How about:
>>>>
>>>> .mplsoudp_label = rte_cpu_to_be32((res->label & 0xfffff) << 12);
>>>>
>>>
>>> Label is 20-bits value in a 24-bits field, see struct rte_flow_item_mpls.
>>
>> OK, I know, what I missed was the following line:
>>
>> rte_memcpy(mplsoudp_encap_conf.label, &id.label[1], 3);
>>
>> Just a suggestion then: using the same memcpy() offsets in both places for
>> clarity:
>>
>> rte_be32_t label = rte_cpu_to_be32(res->label << 12);
>>
>> memcpy(mplsodudp_encap_conf.label, &label, 3);
>>
>> --
>> Adrien Mazarguil
>> 6WIND
@@ -15804,10 +15804,10 @@ static void cmd_set_mplsoudp_encap_parsed(void *parsed_result,
struct cmd_set_mplsoudp_encap_result *res = parsed_result;
union {
uint32_t mplsoudp_label;
- uint8_t label[3];
+ uint8_t label[4];
} id = {
.mplsoudp_label =
- rte_cpu_to_be_32(res->label) & RTE_BE32(0x00ffffff),
+ rte_cpu_to_be_32(res->label<<4) & RTE_BE32(0x00ffffff),
};
if (strcmp(res->mplsoudp, "mplsoudp_encap") == 0)