Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-05 Thread Ilya Maximets 
On 12/4/23 14:18, Marcelo Leitner wrote:
> On Mon, Dec 04, 2023 at 01:57:50PM +0100, Ilya Maximets wrote:
>> On 12/4/23 13:40, Marcelo Leitner wrote:
>>> On Sat, Dec 02, 2023 at 12:06:59AM +0100, Ilya Maximets wrote:
>>> ...
 Adding a test case that demonstrates a scenario where the issue
 occurs - bridging of two tunnels.
>>>
>>> I get the fix and it LGTM. What I don't get is the test case.
>>> Considering that the attr was getting simply ignored, wouldn't the
>>> test case always succeed? That is, I don't see how ignoring tp_src
>>> would cause the test to fail. Can you please elaborate?
>>
>> Ignoring the tp_src is causing tc code to warn about kernel
>> acknowledgement mismatch.  Testsuite doesn't tolerate warnings,
>> so the test fails on checking the logs:
> 
> Nice.
> 
> ...
>> We do not check if the offloading is actually happening, but we
>> check if there is any miscommunication between ovs-vswitchd and
>> the kernel (TC).
>>
>> Does that make sense?
> 
> Yes, thanks.
> 
> Reviewed-by: Marcelo Ricardo Leitner 
> 


Thanks, everyone!
Applied and backported down to 2.17.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-04 Thread Vladislav Odintsov 
Thanks Ilya for the clarification. I didn’t know about check_logs logic.

Tested-by: Vladislav Odintsov 

> On 4 Dec 2023, at 15:59, Ilya Maximets  wrote:
> 
> On 12/2/23 16:16, Vladislav Odintsov wrote:
>> Hi Ilya, thanks for the added test!
>> 
>> I’ve got one question about it, psb.
>> 
>>> On 2 Dec 2023, at 02:06, Ilya Maximets  wrote:
>>> 
>>> There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
>>> should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
>>> by OVS.  Current code just ignores the attribute in the tunnel(set())
>>> action leading to a flow mismatch and potential incorrect datapath
>>> behavior:
>>> 
>>>  |tc(handler21)|DBG|tc flower compare failed action compare
>>>  ...
>>>  Action 0 mismatch:
>>>  - Expected Action:
>>>  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>>>  0020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>>>  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>>>  ...
>>> - Received Action:
>>>  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>>>  0020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>>>  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>>  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>>>  ...
>>> 
>>> In the tc_action dump above we can see the difference on the second
>>> line.  The action dumped from a kernel is missing 'c0 5b' - source
>>> port for a tunnel(set()) action on the second line.
>>> 
>>> Removing the field from the tc_action_encap structure entirely to
>>> avoid any potential confusion.
>>> 
>>> Note: In general, the source port number in the tunnel(set()) action
>>> is not particularly useful for most tunnels, because they may just
>>> ignore the value.  Specs for Geneve and VXLAN suggest using a value
>>> based on the headers of the inner packet as a source port.
>>> In vast majority of scenarios the source port doesn't actually end
>>> up in the action itself.
>>> Having a mismatch between the userspace and TC leads to constant
>>> revalidation of the flow and warnings in the log.
>>> 
>>> Adding a test case that demonstrates a scenario where the issue
>>> occurs - bridging of two tunnels.
>>> 
>>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using 
>>> tc interface")
>>> Reported-at: 
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
>>> Reported-by: Vladislav Odintsov 
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>> 
>>> Version 2:
>>>  * Slightly adjusted a commit message now that we understand the
>>>scenario better.
>>>  * Added a test case that reproduces the issue.
>>> 
>>> 
>>> lib/netdev-offload-tc.c |  4 ++-
>>> lib/tc.h|  3 +-
>>> tests/system-traffic.at | 77 +
>>> 3 files changed, 82 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index b846a63c2..164c7eef6 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, 
>>> struct tc_action *action,
>>> }
>>> break;
>>> case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
>>> -action->encap.tp_src = nl_attr_get_be16(tun_attr);
>>> +/* There is no corresponding attribute in TC. */
>>> +VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
>>> +return EOPNOTSUPP;
>>> }
>>> break;
>>> case OVS_TUNNEL_KEY_ATTR_TP_DST: {
>>> diff --git a/lib/tc.h b/lib/tc.h
>>> index 06707ffa4..fdbcf4b7c 100644
>>> --- a/lib/tc.h
>>> +++ b/lib/tc.h
>>> @@ -213,7 +213,8 @@ enum nat_type {
>>> struct tc_action_encap {
>>> bool id_present;
>>> ovs_be64 id;
>>> -ovs_be16 tp_src;
>>> +/* ovs_be16 tp_src;  Could have been here, but there is no
>>> + * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
>>> ovs_be16 tp_dst;
>>> uint8_t tos;
>>> uint8_t ttl;
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index a7d4ed83b..fd55fdee1 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -903,6 +903,83 @@ ovs-pcap p0.pcap
>>> AT_CHECK([ovs-pcap p0.pcap | grep -Eq 
>>> "^[[[:xdigit:]]]{24}86dd603a1140fc000100fc01[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}6558fa163e949d8008060001080006040001[[[:xdigit:]]]{12}0af40afe$"])
>>> AT_CLEANUP
>>> 
>>> +AT_SETUP([datapath - bridging two geneve tunnels])
>>> +OVS_CHECK_TUNNEL_TSO()
>>> +OVS_CHECK_GENEVE()
>>> +
>>> +OVS_TRAFFIC_VSWITCHD_START()
>>> +ADD_BR([br-underlay-0])
>>> +ADD_BR([br-underlay-1])
>>> +
>>> +ADD_NAMESPACES(at_ns0)
>>> +ADD_NAMESPACES(at_ns1)
>>> +
>>> +dnl Set up underlay link from host into the namespaces using veth pairs.
>>> +ADD_VETH(p0, at_ns0, br-underlay-0,

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-04 Thread Marcelo Leitner 
On Mon, Dec 04, 2023 at 01:57:50PM +0100, Ilya Maximets wrote:
> On 12/4/23 13:40, Marcelo Leitner wrote:
> > On Sat, Dec 02, 2023 at 12:06:59AM +0100, Ilya Maximets wrote:
> > ...
> >> Adding a test case that demonstrates a scenario where the issue
> >> occurs - bridging of two tunnels.
> >
> > I get the fix and it LGTM. What I don't get is the test case.
> > Considering that the attr was getting simply ignored, wouldn't the
> > test case always succeed? That is, I don't see how ignoring tp_src
> > would cause the test to fail. Can you please elaborate?
>
> Ignoring the tp_src is causing tc code to warn about kernel
> acknowledgement mismatch.  Testsuite doesn't tolerate warnings,
> so the test fails on checking the logs:

Nice.

...
> We do not check if the offloading is actually happening, but we
> check if there is any miscommunication between ovs-vswitchd and
> the kernel (TC).
>
> Does that make sense?

Yes, thanks.

Reviewed-by: Marcelo Ricardo Leitner 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-04 Thread Ilya Maximets 
On 12/2/23 16:16, Vladislav Odintsov wrote:
> Hi Ilya, thanks for the added test!
> 
> I’ve got one question about it, psb.
> 
>> On 2 Dec 2023, at 02:06, Ilya Maximets  wrote:
>>
>> There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
>> should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
>> by OVS.  Current code just ignores the attribute in the tunnel(set())
>> action leading to a flow mismatch and potential incorrect datapath
>> behavior:
>>
>>  |tc(handler21)|DBG|tc flower compare failed action compare
>>  ...
>>  Action 0 mismatch:
>>  - Expected Action:
>>  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>>  0020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>>  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>>  ...
>> - Received Action:
>>  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>>  0020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>>  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>>  ...
>>
>> In the tc_action dump above we can see the difference on the second
>> line.  The action dumped from a kernel is missing 'c0 5b' - source
>> port for a tunnel(set()) action on the second line.
>>
>> Removing the field from the tc_action_encap structure entirely to
>> avoid any potential confusion.
>>
>> Note: In general, the source port number in the tunnel(set()) action
>> is not particularly useful for most tunnels, because they may just
>> ignore the value.  Specs for Geneve and VXLAN suggest using a value
>> based on the headers of the inner packet as a source port.
>> In vast majority of scenarios the source port doesn't actually end
>> up in the action itself.
>> Having a mismatch between the userspace and TC leads to constant
>> revalidation of the flow and warnings in the log.
>>
>> Adding a test case that demonstrates a scenario where the issue
>> occurs - bridging of two tunnels.
>>
>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc 
>> interface")
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
>> Reported-by: Vladislav Odintsov 
>> Signed-off-by: Ilya Maximets 
>> ---
>>
>> Version 2:
>>  * Slightly adjusted a commit message now that we understand the
>>    scenario better.
>>  * Added a test case that reproduces the issue.
>>
>>
>> lib/netdev-offload-tc.c |  4 ++-
>> lib/tc.h    |  3 +-
>> tests/system-traffic.at | 77 +
>> 3 files changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index b846a63c2..164c7eef6 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, 
>> struct tc_action *action,
>> }
>> break;
>> case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
>> -    action->encap.tp_src = nl_attr_get_be16(tun_attr);
>> +    /* There is no corresponding attribute in TC. */
>> +    VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
>> +    return EOPNOTSUPP;
>> }
>> break;
>> case OVS_TUNNEL_KEY_ATTR_TP_DST: {
>> diff --git a/lib/tc.h b/lib/tc.h
>> index 06707ffa4..fdbcf4b7c 100644
>> --- a/lib/tc.h
>> +++ b/lib/tc.h
>> @@ -213,7 +213,8 @@ enum nat_type {
>> struct tc_action_encap {
>> bool id_present;
>> ovs_be64 id;
>> -    ovs_be16 tp_src;
>> +    /* ovs_be16 tp_src;  Could have been here, but there is no
>> + * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
>> ovs_be16 tp_dst;
>> uint8_t tos;
>> uint8_t ttl;
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index a7d4ed83b..fd55fdee1 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -903,6 +903,83 @@ ovs-pcap p0.pcap
>> AT_CHECK([ovs-pcap p0.pcap | grep -Eq 
>> "^[[[:xdigit:]]]{24}86dd603a1140fc000100fc01[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}6558fa163e949d8008060001080006040001[[[:xdigit:]]]{12}0af40afe$"])
>> AT_CLEANUP
>>
>> +AT_SETUP([datapath - bridging two geneve tunnels])
>> +OVS_CHECK_TUNNEL_TSO()
>> +OVS_CHECK_GENEVE()
>> +
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-underlay-0])
>> +ADD_BR([br-underlay-1])
>> +
>> +ADD_NAMESPACES(at_ns0)
>> +ADD_NAMESPACES(at_ns1)
>> +
>> +dnl Set up underlay link from host into the namespaces using veth pairs.
>> +ADD_VETH(p0, at_ns0, br-underlay-0, "172.31.1.1/24")
>> +AT_CHECK([ip addr add dev br-underlay-0 "172.31.1.100/24"])
>> +AT_CHECK([ip link set dev br-underlay-0 up])
>> +
>> +ADD_VETH(p1, at_ns1, br-underlay-1, "172.31.2.1/24")
>> +AT_CHECK([ip addr add dev br-underlay-1 "172.31.2.100/24"])
>> +AT_CHECK([ip link set dev br-underlay-1 up])

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-04 Thread Ilya Maximets 
On 12/4/23 13:40, Marcelo Leitner wrote:
> On Sat, Dec 02, 2023 at 12:06:59AM +0100, Ilya Maximets wrote:
> ...
>> Adding a test case that demonstrates a scenario where the issue
>> occurs - bridging of two tunnels.
> 
> I get the fix and it LGTM. What I don't get is the test case.
> Considering that the attr was getting simply ignored, wouldn't the
> test case always succeed? That is, I don't see how ignoring tp_src
> would cause the test to fail. Can you please elaborate?

Ignoring the tp_src is causing tc code to warn about kernel
acknowledgement mismatch.  Testsuite doesn't tolerate warnings,
so the test fails on checking the logs:


./system-traffic.at:980: check_logs 
--- /dev/null   2023-11-16 11:36:50.99100 -0500
+++ /root/ovs/tests/system-offloads-testsuite.dir/at-groups/40/stdout   
2023-12-04 07:55:35.055945362 -0500
@@ -0,0 +1,6 @@
+2023-12-04T12:55:32.199Z|1|tc(handler22)|WARN|Kernel flower acknowledgment 
does not match request!  Set dpif_netlink to dbg to see which rule caused this 
error.
+2023-12-04T12:55:32.199Z|2|tc(handler22)|WARN|Kernel flower acknowledgment 
does not match request!  Set dpif_netlink to dbg to see which rule caused this 
error.
+2023-12-04T12:55:32.574Z|1|tc(handler8)|WARN|Kernel flower acknowledgment 
does not match request!  Set dpif_netlink to dbg to see which rule caused this 
error.
+2023-12-04T12:55:32.765Z|1|tc(handler20)|WARN|Kernel flower acknowledgment 
does not match request!  Set dpif_netlink to dbg to see which rule caused this 
error.
+2023-12-04T12:55:33.189Z|1|tc(handler23)|WARN|Kernel flower acknowledgment 
does not match request!  Set dpif_netlink to dbg to see which rule caused this 
error.
+2023-12-04T12:55:33.597Z|3|tc(handler8)|WARN|Kernel flower acknowledgment 
does not match request!  Set dpif_netlink to dbg to see which rule caused this 
error.
<...>
40. system-traffic.at:906:  FAILED (system-traffic.at:980)

We do not check if the offloading is actually happening, but we
check if there is any miscommunication between ovs-vswitchd and
the kernel (TC).

Does that make sense?

> 
>   Marcelo
> 
>>
>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc 
>> interface")
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
>> Reported-by: Vladislav Odintsov 
>> Signed-off-by: Ilya Maximets 
>> ---
>>
>> Version 2:
>>   * Slightly adjusted a commit message now that we understand the
>> scenario better.
>>   * Added a test case that reproduces the issue.
>>
>>
>>  lib/netdev-offload-tc.c |  4 ++-
>>  lib/tc.h|  3 +-
>>  tests/system-traffic.at | 77 +
>>  3 files changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index b846a63c2..164c7eef6 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, 
>> struct tc_action *action,
>>  }
>>  break;
>>  case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
>> -action->encap.tp_src = nl_attr_get_be16(tun_attr);
>> +/* There is no corresponding attribute in TC. */
>> +VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
>> +return EOPNOTSUPP;
>>  }
>>  break;
>>  case OVS_TUNNEL_KEY_ATTR_TP_DST: {
>> diff --git a/lib/tc.h b/lib/tc.h
>> index 06707ffa4..fdbcf4b7c 100644
>> --- a/lib/tc.h
>> +++ b/lib/tc.h
>> @@ -213,7 +213,8 @@ enum nat_type {
>>  struct tc_action_encap {
>>  bool id_present;
>>  ovs_be64 id;
>> -ovs_be16 tp_src;
>> +/* ovs_be16 tp_src;  Could have been here, but there is no
>> + * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
>>  ovs_be16 tp_dst;
>>  uint8_t tos;
>>  uint8_t ttl;
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index a7d4ed83b..fd55fdee1 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -903,6 +903,83 @@ ovs-pcap p0.pcap
>>  AT_CHECK([ovs-pcap p0.pcap | grep -Eq 
>> "^[[[:xdigit:]]]{24}86dd603a1140fc000100fc01[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}6558fa163e949d8008060001080006040001[[[:xdigit:]]]{12}0af40afe$"])
>>  AT_CLEANUP
>>
>> +AT_SETUP([datapath - bridging two geneve tunnels])
>> +OVS_CHECK_TUNNEL_TSO()
>> +OVS_CHECK_GENEVE()
>> +
>> +OVS_TRAFFIC_VSWITCHD_START()
>> +ADD_BR([br-underlay-0])
>> +ADD_BR([br-underlay-1])
>> +
>> +ADD_NAMESPACES(at_ns0)
>> +ADD_NAMESPACES(at_ns1)
>> +
>> +dnl Set up underlay link from host into the namespaces using veth pairs.
>> +ADD_VETH(p0, at_ns0, br-underlay-0, "172.31.1.1/24")
>> +AT_CHECK([ip addr add dev br-underlay-0 "172.31.1.100/24"])
>> +AT_CHECK([ip link set dev br-underlay-0 up])
>> +
>> +ADD_VETH(p1, at_ns1, br-underlay-1, "172.31.2.1/24")
>>

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-04 Thread Marcelo Leitner 
On Sat, Dec 02, 2023 at 12:06:59AM +0100, Ilya Maximets wrote:
...
> Adding a test case that demonstrates a scenario where the issue
> occurs - bridging of two tunnels.

I get the fix and it LGTM. What I don't get is the test case.
Considering that the attr was getting simply ignored, wouldn't the
test case always succeed? That is, I don't see how ignoring tp_src
would cause the test to fail. Can you please elaborate?

  Marcelo

>
> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc 
> interface")
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
> Reported-by: Vladislav Odintsov 
> Signed-off-by: Ilya Maximets 
> ---
>
> Version 2:
>   * Slightly adjusted a commit message now that we understand the
> scenario better.
>   * Added a test case that reproduces the issue.
>
>
>  lib/netdev-offload-tc.c |  4 ++-
>  lib/tc.h|  3 +-
>  tests/system-traffic.at | 77 +
>  3 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index b846a63c2..164c7eef6 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, 
> struct tc_action *action,
>  }
>  break;
>  case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
> -action->encap.tp_src = nl_attr_get_be16(tun_attr);
> +/* There is no corresponding attribute in TC. */
> +VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
> +return EOPNOTSUPP;
>  }
>  break;
>  case OVS_TUNNEL_KEY_ATTR_TP_DST: {
> diff --git a/lib/tc.h b/lib/tc.h
> index 06707ffa4..fdbcf4b7c 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -213,7 +213,8 @@ enum nat_type {
>  struct tc_action_encap {
>  bool id_present;
>  ovs_be64 id;
> -ovs_be16 tp_src;
> +/* ovs_be16 tp_src;  Could have been here, but there is no
> + * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
>  ovs_be16 tp_dst;
>  uint8_t tos;
>  uint8_t ttl;
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index a7d4ed83b..fd55fdee1 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -903,6 +903,83 @@ ovs-pcap p0.pcap
>  AT_CHECK([ovs-pcap p0.pcap | grep -Eq 
> "^[[[:xdigit:]]]{24}86dd603a1140fc000100fc01[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}6558fa163e949d8008060001080006040001[[[:xdigit:]]]{12}0af40afe$"])
>  AT_CLEANUP
>
> +AT_SETUP([datapath - bridging two geneve tunnels])
> +OVS_CHECK_TUNNEL_TSO()
> +OVS_CHECK_GENEVE()
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-underlay-0])
> +ADD_BR([br-underlay-1])
> +
> +ADD_NAMESPACES(at_ns0)
> +ADD_NAMESPACES(at_ns1)
> +
> +dnl Set up underlay link from host into the namespaces using veth pairs.
> +ADD_VETH(p0, at_ns0, br-underlay-0, "172.31.1.1/24")
> +AT_CHECK([ip addr add dev br-underlay-0 "172.31.1.100/24"])
> +AT_CHECK([ip link set dev br-underlay-0 up])
> +
> +ADD_VETH(p1, at_ns1, br-underlay-1, "172.31.2.1/24")
> +AT_CHECK([ip addr add dev br-underlay-1 "172.31.2.100/24"])
> +AT_CHECK([ip link set dev br-underlay-1 up])
> +
> +dnl Set up two OVS tunnel endpoints in a root namespace and two native
> +dnl linux devices inside the test namespaces.
> +dnl
> +dnl  ns_gnv0  |   ns_gnv1
> +dnl  ip:10.1.1.1/24   |   ip:10.1.1.2/24
> +dnl  remote_ip: 172.31.1.100  |   remote_ip: 172.31.2.100
> +dnl  ||  |
> +dnl  ||  |
> +dnl  p0   |   p1
> +dnl  ip: 172.31.1.1/24|   ip: 172.31.2.1/24
> +dnl  |  NS0   |   NS1|
> +dnl 
> -|+--|
> +dnl  |   |
> +dnl   br-underlay-0:   br-underlay-1:
> +dnl   ip: 172.31.1.100/24  ip: 172.31.2.100/24
> +dnl ovs-p0   ovs-p1
> +dnl  |   |
> +dnl  |br0|
> +dnl   encap/decap --- ip: 10.1.1.100/24 - encap/decap
> +dnl at_gnv0
> +dnl   remote_ip: 172.31.1.1
> +dnl at_gnv1
> +dnl   remote_ip: 172.31.2.1
> +dnl
> +ADD_OVS_TUNNEL([geneve], [br0], [at_gnv0], [172.31.1.1], [10.1.1.100/24])
> +ADD_NATIVE_TUNNEL([geneve], [ns_gnv0], [at_ns0], [172.31.1.100], 
> [10.1.1.1/24],
> +  [vni 0])
> +ADD_OVS_TUNNEL([geneve], [br0],

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-04 Thread Eelco Chaudron 



On 2 Dec 2023, at 0:06, Ilya Maximets wrote:

> There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
> should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
> by OVS.  Current code just ignores the attribute in the tunnel(set())
> action leading to a flow mismatch and potential incorrect datapath
> behavior:
>
>   |tc(handler21)|DBG|tc flower compare failed action compare
>   ...
>   Action 0 mismatch:
>   - Expected Action:
>   0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>   0020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>   0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>   ...
>  - Received Action:
>   0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>   0020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>   0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>   ...
>
> In the tc_action dump above we can see the difference on the second
> line.  The action dumped from a kernel is missing 'c0 5b' - source
> port for a tunnel(set()) action on the second line.
>
> Removing the field from the tc_action_encap structure entirely to
> avoid any potential confusion.
>
> Note: In general, the source port number in the tunnel(set()) action
> is not particularly useful for most tunnels, because they may just
> ignore the value.  Specs for Geneve and VXLAN suggest using a value
> based on the headers of the inner packet as a source port.
> In vast majority of scenarios the source port doesn't actually end
> up in the action itself.
> Having a mismatch between the userspace and TC leads to constant
> revalidation of the flow and warnings in the log.
>
> Adding a test case that demonstrates a scenario where the issue
> occurs - bridging of two tunnels.
>
> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc 
> interface")
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
> Reported-by: Vladislav Odintsov 
> Signed-off-by: Ilya Maximets 
> ---

Thank for fixing this and adding the unit test.

Acked-by: Eelco Chaudron 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-02 Thread Vladislav Odintsov 
Hi Ilya, thanks for the added test!

I’ve got one question about it, psb.

> On 2 Dec 2023, at 02:06, Ilya Maximets  wrote:
> 
> There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
> should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
> by OVS.  Current code just ignores the attribute in the tunnel(set())
> action leading to a flow mismatch and potential incorrect datapath
> behavior:
> 
>  |tc(handler21)|DBG|tc flower compare failed action compare
>  ...
>  Action 0 mismatch:
>  - Expected Action:
>  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>  0020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>  ...
> - Received Action:
>  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
>  0020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
>  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
>  ...
> 
> In the tc_action dump above we can see the difference on the second
> line.  The action dumped from a kernel is missing 'c0 5b' - source
> port for a tunnel(set()) action on the second line.
> 
> Removing the field from the tc_action_encap structure entirely to
> avoid any potential confusion.
> 
> Note: In general, the source port number in the tunnel(set()) action
> is not particularly useful for most tunnels, because they may just
> ignore the value.  Specs for Geneve and VXLAN suggest using a value
> based on the headers of the inner packet as a source port.
> In vast majority of scenarios the source port doesn't actually end
> up in the action itself.
> Having a mismatch between the userspace and TC leads to constant
> revalidation of the flow and warnings in the log.
> 
> Adding a test case that demonstrates a scenario where the issue
> occurs - bridging of two tunnels.
> 
> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc 
> interface")
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
> Reported-by: Vladislav Odintsov 
> Signed-off-by: Ilya Maximets 
> ---
> 
> Version 2:
>  * Slightly adjusted a commit message now that we understand the
>scenario better.
>  * Added a test case that reproduces the issue.
> 
> 
> lib/netdev-offload-tc.c |  4 ++-
> lib/tc.h|  3 +-
> tests/system-traffic.at | 77 +
> 3 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index b846a63c2..164c7eef6 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, 
> struct tc_action *action,
> }
> break;
> case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
> -action->encap.tp_src = nl_attr_get_be16(tun_attr);
> +/* There is no corresponding attribute in TC. */
> +VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
> +return EOPNOTSUPP;
> }
> break;
> case OVS_TUNNEL_KEY_ATTR_TP_DST: {
> diff --git a/lib/tc.h b/lib/tc.h
> index 06707ffa4..fdbcf4b7c 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -213,7 +213,8 @@ enum nat_type {
> struct tc_action_encap {
> bool id_present;
> ovs_be64 id;
> -ovs_be16 tp_src;
> +/* ovs_be16 tp_src;  Could have been here, but there is no
> + * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
> ovs_be16 tp_dst;
> uint8_t tos;
> uint8_t ttl;
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index a7d4ed83b..fd55fdee1 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -903,6 +903,83 @@ ovs-pcap p0.pcap
> AT_CHECK([ovs-pcap p0.pcap | grep -Eq 
> "^[[[:xdigit:]]]{24}86dd603a1140fc000100fc01[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}6558fa163e949d8008060001080006040001[[[:xdigit:]]]{12}0af40afe$"])
> AT_CLEANUP
> 
> +AT_SETUP([datapath - bridging two geneve tunnels])
> +OVS_CHECK_TUNNEL_TSO()
> +OVS_CHECK_GENEVE()
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-underlay-0])
> +ADD_BR([br-underlay-1])
> +
> +ADD_NAMESPACES(at_ns0)
> +ADD_NAMESPACES(at_ns1)
> +
> +dnl Set up underlay link from host into the namespaces using veth pairs.
> +ADD_VETH(p0, at_ns0, br-underlay-0, "172.31.1.1/24")
> +AT_CHECK([ip addr add dev br-underlay-0 "172.31.1.100/24"])
> +AT_CHECK([ip link set dev br-underlay-0 up])
> +
> +ADD_VETH(p1, at_ns1, br-underlay-1, "172.31.2.1/24")
> +AT_CHECK([ip addr add dev br-underlay-1 "172.31.2.100/24"])
> +AT_CHECK([ip link set dev br-underlay-1 up])
> +
> +dnl Set up two OVS tunnel endpoints in a root namespace and two native
> +dnl linux devices inside the test namespaces.
> +dnl
> +dnl  ns_gnv0

[ovs-dev] [PATCH v2] netdev-offload-tc: Fix offload of tunnel key tp_src.

2023-12-01 Thread Ilya Maximets 
There is no TCA_TUNNEL_KEY_ENC_SRC_PORT in the kernel, so the offload
should not be attempted if OVS_TUNNEL_KEY_ATTR_TP_SRC is requested
by OVS.  Current code just ignores the attribute in the tunnel(set())
action leading to a flow mismatch and potential incorrect datapath
behavior:

  |tc(handler21)|DBG|tc flower compare failed action compare
  ...
  Action 0 mismatch:
  - Expected Action:
  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
  0020 c0 5b 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
  ...
 - Received Action:
  0010 01 00 00 00 00 00 00 00-00 00 00 00 00 ff 00 11
  0020 00 00 17 c1 00 40 00 00-0a 01 00 6d 0a 01 01 12
  0050 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0060 01 02 80 01 00 18 00 0b-00 00 00 00 00 00 00 00
  ...

In the tc_action dump above we can see the difference on the second
line.  The action dumped from a kernel is missing 'c0 5b' - source
port for a tunnel(set()) action on the second line.

Removing the field from the tc_action_encap structure entirely to
avoid any potential confusion.

Note: In general, the source port number in the tunnel(set()) action
is not particularly useful for most tunnels, because they may just
ignore the value.  Specs for Geneve and VXLAN suggest using a value
based on the headers of the inner packet as a source port.
In vast majority of scenarios the source port doesn't actually end
up in the action itself.
Having a mismatch between the userspace and TC leads to constant
revalidation of the flow and warnings in the log.

Adding a test case that demonstrates a scenario where the issue
occurs - bridging of two tunnels.

Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc 
interface")
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2023-October/052744.html
Reported-by: Vladislav Odintsov 
Signed-off-by: Ilya Maximets 
---

Version 2:
  * Slightly adjusted a commit message now that we understand the
scenario better.
  * Added a test case that reproduces the issue.


 lib/netdev-offload-tc.c |  4 ++-
 lib/tc.h|  3 +-
 tests/system-traffic.at | 77 +
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index b846a63c2..164c7eef6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1627,7 +1627,9 @@ parse_put_flow_set_action(struct tc_flower *flower, 
struct tc_action *action,
 }
 break;
 case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
-action->encap.tp_src = nl_attr_get_be16(tun_attr);
+/* There is no corresponding attribute in TC. */
+VLOG_DBG_RL(, "unsupported tunnel key attribute TP_SRC");
+return EOPNOTSUPP;
 }
 break;
 case OVS_TUNNEL_KEY_ATTR_TP_DST: {
diff --git a/lib/tc.h b/lib/tc.h
index 06707ffa4..fdbcf4b7c 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -213,7 +213,8 @@ enum nat_type {
 struct tc_action_encap {
 bool id_present;
 ovs_be64 id;
-ovs_be16 tp_src;
+/* ovs_be16 tp_src;  Could have been here, but there is no
+ * TCA_TUNNEL_KEY_ENC_ attribute for it in the kernel. */
 ovs_be16 tp_dst;
 uint8_t tos;
 uint8_t ttl;
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index a7d4ed83b..fd55fdee1 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -903,6 +903,83 @@ ovs-pcap p0.pcap
 AT_CHECK([ovs-pcap p0.pcap | grep -Eq 
"^[[[:xdigit:]]]{24}86dd603a1140fc000100fc01[[[:xdigit:]]]{4}17c1003a[[[:xdigit:]]]{4}6558fa163e949d8008060001080006040001[[[:xdigit:]]]{12}0af40afe$"])
 AT_CLEANUP
 
+AT_SETUP([datapath - bridging two geneve tunnels])
+OVS_CHECK_TUNNEL_TSO()
+OVS_CHECK_GENEVE()
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-underlay-0])
+ADD_BR([br-underlay-1])
+
+ADD_NAMESPACES(at_ns0)
+ADD_NAMESPACES(at_ns1)
+
+dnl Set up underlay link from host into the namespaces using veth pairs.
+ADD_VETH(p0, at_ns0, br-underlay-0, "172.31.1.1/24")
+AT_CHECK([ip addr add dev br-underlay-0 "172.31.1.100/24"])
+AT_CHECK([ip link set dev br-underlay-0 up])
+
+ADD_VETH(p1, at_ns1, br-underlay-1, "172.31.2.1/24")
+AT_CHECK([ip addr add dev br-underlay-1 "172.31.2.100/24"])
+AT_CHECK([ip link set dev br-underlay-1 up])
+
+dnl Set up two OVS tunnel endpoints in a root namespace and two native
+dnl linux devices inside the test namespaces.
+dnl
+dnl  ns_gnv0  |   ns_gnv1
+dnl  ip:10.1.1.1/24   |   ip:10.1.1.2/24
+dnl  remote_ip: 172.31.1.100  |   remote_ip: 172.31.2.100
+dnl  ||  |
+dnl  ||  |
+dnl  p0