[ovs-dev] [PATCH 01/11] netdev-offload-dpdk: Remove pre-validate of patterns function

Eli Britstein elibr at mellanox.com
Wed May 27 04:41:33 UTC 2020


On 5/26/2020 11:30 PM, Ilya Maximets wrote:
> On 5/26/20 3:46 PM, Eli Britstein wrote:
>> On 5/19/2020 5:26 PM, Eli Britstein wrote:
>>> On 5/19/2020 4:41 PM, Sriharsha Basavapatna wrote:
>>>> On Mon, May 18, 2020 at 9:10 PM Eli Britstein <elibr at mellanox.com> wrote:
>>>>> The function of adding patterns by requested matches checks that it
>>>>> consumed all the required matches, and err if not. This nullify the
>>>>> purpose of the validation function. Future supported matches will only
>>>>> change the pattern parsing code.
>>>>>
>>>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>>>> Reviewed-by: Roni Bar Yanai <roniba at mellanox.com>
>>>>> ---
>>>>>    lib/netdev-offload-dpdk.c | 133 ++++++++++++++++++----------------------------
>>>>>    1 file changed, 51 insertions(+), 82 deletions(-)
>>>>>
>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>>> index f8c46bbaa..84bbe29e7 100644
>>>>> --- a/lib/netdev-offload-dpdk.c
>>>>> +++ b/lib/netdev-offload-dpdk.c
>>>>> @@ -559,10 +559,22 @@ free_flow_actions(struct flow_actions *actions)
>>>>>
>>>>>    static int
>>>>>    parse_flow_match(struct flow_patterns *patterns,
>>>>> -                 const struct match *match)
>>>>> +                 struct match *match)
>>>>>    {
>>>>>        uint8_t *next_proto_mask = NULL;
>>>>> +    struct flow *consumed_masks;
>>>>>        uint8_t proto = 0;
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    consumed_masks = &match->wc.masks;
>>>>> +
>>>>> +    memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
>>>>> +    if (match->flow.recirc_id != 0) {
>>>>> +        ret = -1;
>>>>> +        goto out;
>>>>> +    }
>>>>> +    consumed_masks->recirc_id = 0;
>>>>> +    consumed_masks->packet_type = 0;
>>>>>
>>>>>        /* Eth */
>>>>>        if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>>>> @@ -580,6 +592,9 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>>            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);
>>>>> +
>>>>>            add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask);
>>>>>        } else {
>>>>>            /*
>>>>> @@ -591,6 +606,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>>             */
>>>>>            add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>>>>>        }
>>>>> +    consumed_masks->dl_type = 0;
>>>> Why not set the dl_type also under the if() condition above, where
>>>> dl_src an dl_dst consumed masks are being set ? Similarly, vlan
>>>> consumed mask below.
>>> OVS always matches on dl_type (same for the below of VLAN). We must clean it in any case in order not to fail.
>> That's right. It's a bug here from upstream. dl_type should be handled even if no matches on src/dst. I'll add a commit to fix it in v2.
> Is it something related:
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fopenvswitch%2Fpatch%2F1587180266-28632-1-git-send-email-JackerDune%40gmail.com%2F&data=02%7C01%7Celibr%40mellanox.com%7C1598511210834cb7bfeb08d801b39c95%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637261218225476915&sdata=JCsdv1rqjKljOQh%2F9%2BD53lbQMq0NYAqqzph08JO5y9Y%3D&reserved=0
> ?
Yes. However, this patch causes this case not to be offloaded at all, 
with the claim that Intel XL710 doesn't support it. As OVS is vendor 
agnostic, such workarounds should not be in OVS, but rather in the 
specific PMD code. My fix will add offload support for this case, and 
not reject it.
>
> Best regards, Ilya Maximets.


More information about the dev mailing list