[ovs-dev] [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710 NIC
Ilya Maximets
i.maximets at ovn.org
Fri Aug 14 12:57:33 UTC 2020
On 8/14/20 2:29 PM, Stokes, Ian wrote:
>> On 8/14/20 10:58 AM, Stokes, Ian wrote:
>>>>> -----Original Message-----
>>>>> From: Ilya Maximets <i.maximets at ovn.org>
>>>>> Sent: Thursday, August 13, 2020 9:26 PM
>>>>> To: Emma Finn <emma.finn at intel.com>; dev at openvswitch.org
>>>>> Cc: ian.stokes at intel.com; i.maximets at ovn.org; Eli Britstein
>>>>> <elibr at nvidia.com>; beilei.xing at intel.com; i.maximets at ovn.org
>>>>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet
>>>>> matching HWOL for XL710 NIC
>>>>>
>>>>>
>>>>> On 8/13/20 6:13 PM, Emma Finn wrote:
>>>>>> This patch introduces a temporary work around to fix partial
>>>>>> hardware offload for XL710 devices. Currently the incorrect
>>>>>> ethernet pattern is being set. This patch will be removed once this
>>>>>> issue is fixed within the i40e PMD.
>>>>>>
>>>>>> Signed-off-by: Emma Finn <emma.finn at intel.com>
>>>>>> Signed-off-by: Eli Britstein <elibr at nvidia.com>
>>>>>> Co-authored-by: Eli Britstein <elibr at nvidia.com>
>>>>>> ---
>>>>>> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++----------
>>>>>> 1 file changed, 25 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>>>> index de6101e..ede2077 100644
>>>>>> --- a/lib/netdev-offload-dpdk.c
>>>>>> +++ b/lib/netdev-offload-dpdk.c
>>>>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions)
>>>>>> actions->cnt = 0;
>>>>>> }
>>>>>>
>>>>>> +/*
>>>>>> +* This is a temporary work around to fix ethernet pattern for
>>>>>> +partial hardware
>>>>>> +* offload for X710 devices. This fix will be reverted once the
>>>>>> +issue is fixed
>>>>>> +* within the i40e PMD driver.
>>>>>> +*/
>>>>>> +#define NULL_ETH_MASK_IF_ZEROS \
>>>>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src)
>> && \
>>>>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \
>>>>>> + patterns->items[0].mask = NULL; \
>>>>>> + free(eth_mask); \
>>>>>> + }
>>>>>
>>>>> Could you please address my comments from this e-mail:
>>>>>
>>>>>
>> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.
>>>>> op
>>>>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020-
>>>>>
>> August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f
>>>>>
>> a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a%
>>>>>
>> 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y
>>>>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0
>>>>> ?
>>>>>
>>>>> i.e. convert this macro definition into function.
>>>> My previous patch was a POC. Here is a more "nicer" one. No
>>>> macro/function, and no redundant allocation/free:
>>>> Need to test and obviously finalize.
>>>
>>> Thanks for this Eli, just a heads up Emma is out of office today but I can test
>> this and submit the v2 if you prefer?
>>>
>>> @Ilya, what are your thoughts on this approach below?
>>
>> It looks fine, but I'd re-write the if statement in a following way:
>>
>> if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow)
>
> Just a quick one here Ilya, sparse throws an restricted ovs_be16 degrades to integer error with the above line, are you ok with the following change
>
> if (match->wc.masks.dl_type == htons(0xFFFF) && is_ip_any(&match->flow)
Sure. I'd prefer keep the lowercase for a constant, but that is not really important.
>
> BR
> Ian
>> && eth_addr_is_zero(match->wc.masks.dl_dst)
>> && eth_addr_is_zero(match->wc.masks.dl_src)) {
>>
>> i.e. lightweight operations first, special functions for the flow type and eth_addr
>> checking.
>>
>>>
>>> BR
>>> Ian
>>>>
>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>> index
>>>> de6101e4d..c6d293af3 100644
>>>> --- a/lib/netdev-offload-dpdk.c
>>>> +++ b/lib/netdev-offload-dpdk.c
>>>> @@ -696,16 +696,26 @@ parse_flow_match(struct flow_patterns *patterns,
>>>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
>>>> struct rte_flow_item_eth *spec, *mask;
>>>>
>>>> - spec = xzalloc(sizeof *spec);
>>>> - mask = xzalloc(sizeof *mask);
>>>> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */
>>>> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) &&
>>>> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) &&
>>>> + match->wc.masks.dl_type == 0xFFFF &&
>>>> + (match->flow.dl_type == htons(ETH_TYPE_IP) ||
>>>> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) {
>>>> + spec = NULL;
>>>> + mask = NULL;
>>>> + } else {
>>>> + spec = xzalloc(sizeof *spec);
>>>> + mask = xzalloc(sizeof *mask);
>>>>
>>>> - memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
>>>> - memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
>>>> - spec->type = match->flow.dl_type;
>>>> + memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
>>>> + memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
>>>> + spec->type = match->flow.dl_type;
>>>>
>>>> - memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
>>>> - memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
>>>> - mask->type = match->wc.masks.dl_type;
>>>> + memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
>>>> + memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
>>>> + mask->type = match->wc.masks.dl_type;
>>>> + }
>>>>
>>>> memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks-
>>> dl_dst);
>>>> memset(&consumed_masks->dl_src, 0, sizeof
>>>> consumed_masks->dl_src);
>>>>>
>>>>>> +
>>>>>> static int
>>>>>> parse_flow_match(struct flow_patterns *patterns,
>>>>>> struct match *match) {
>>>>>> + struct rte_flow_item_eth *eth_mask = NULL;
>>>>>> + struct rte_flow_item_eth *eth_spec = NULL;
>>>>>> uint8_t *next_proto_mask = NULL;
>>>>>> struct flow *consumed_masks;
>>>>>> uint8_t proto = 0;
>>>>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns
>> *patterns,
>>>>>> if (match->wc.masks.dl_type ||
>>>>>> !eth_addr_is_zero(match->wc.masks.dl_src) ||
>>>>>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
>>>>>> - struct rte_flow_item_eth *spec, *mask;
>>>>>>
>>>>>> - spec = xzalloc(sizeof *spec);
>>>>>> - mask = xzalloc(sizeof *mask);
>>>>>> + eth_spec = xzalloc(sizeof *eth_spec);
>>>>>> + eth_mask = xzalloc(sizeof *eth_mask);
>>>>>>
>>>>>> - memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
>>>>>> - memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
>>>>>> - spec->type = match->flow.dl_type;
>>>>>> + memcpy(ð_spec->dst, &match->flow.dl_dst, sizeof eth_spec-
>>> dst);
>>>>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec-
>>> src);
>>>>>> + eth_spec->type = match->flow.dl_type;
>>>>>>
>>>>>> - memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
>>>>>> - memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
>>>>>> - mask->type = match->wc.masks.dl_type;
>>>>>> + memcpy(ð_mask->dst, &match->wc.masks.dl_dst, sizeof
>>>>>> + eth_mask-
>>>>>> dst);
>>>>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof
>>>>>> + eth_mask-
>>>>>> src);
>>>>>> + eth_mask->type = match->wc.masks.dl_type;
>>>>>>
>>>>>> memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks-
>>>>> dl_dst);
>>>>>> memset(&consumed_masks->dl_src, 0, sizeof consumed_masks-
>>>>> dl_src);
>>>>>> consumed_masks->dl_type = 0;
>>>>>>
>>>>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec,
>> mask);
>>>>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH,
>>>>>> + eth_spec, eth_mask);
>>>>>> }
>>>>>>
>>>>>> /* VLAN */
>>>>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>>> /* IP v4 */
>>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
>>>>>> struct rte_flow_item_ipv4 *spec, *mask;
>>>>>
>>>>> Empty line needed.
>>>>>
>>>>>> + NULL_ETH_MASK_IF_ZEROS;
>>>>>>
>>>>>> spec = xzalloc(sizeof *spec);
>>>>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@
>>>>>> parse_flow_match(struct flow_patterns *patterns,
>>>>>> /* IP v6 */
>>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) {
>>>>>> struct rte_flow_item_ipv6 *spec, *mask;
>>>>>
>>>>> ditto.
>>>>>
>>>>>> + NULL_ETH_MASK_IF_ZEROS;
>>>>>>
>>>>>> spec = xzalloc(sizeof *spec);
>>>>>> mask = xzalloc(sizeof *mask);
>>>>>>
>>>
>
More information about the dev
mailing list