[ovs-dev] [PATCH] netdev-offload-dpdk: Pass L4 proto-id to match in the L3 rte_flow_item

[Sriharsha Basavapatna]

On Fri, Jul 10, 2020 at 4:01 PM Sriharsha Basavapatna <
sriharsha.basavapatna at broadcom.com> wrote:

>
>
> On Sun, Jul 5, 2020 at 6:00 PM Eli Britstein <elibr at mellanox.com> wrote:
>
>>
>> On 7/5/2020 2:48 PM, Sriharsha Basavapatna wrote:
>> > The offload layer clears the L4 protocol mask in the L3 item, when the
>> > L4 item is passed for matching, as an optimization. This can be
>> confusing
>> > while parsing the headers in the PMD. Also, the datapath flow specifies
>> > this field to be matched. This optimization is best left to the PMD.
>> > This patch restores the code to pass the L4 protocol type in L3 match.
>> >
>> > Fixes: 900fe00784ca ("netdev-offload-dpdk: Dynamically allocate pattern
>> items.")
>>
>> It's arguable if it's really a fix.
>
> It is better not to ignore a field that is specified to be matched by the
> datapath flow.
>
>
>> I don't see any further information
>> the PMD can use, but it's harmless anyway, so OK by me either with this
>> commit or without.
>
> If you insist it's a fix, this is the correct commit that did it in the
>> first place:
>>
>> e8a2b5bf92bb netdev-dpdk: implement flow offload with rte flow
>>
>
> Thanks, I'll update the "fixes" field in v2.
> -Harsha
>

I'll send v2 of this patch in a patchset with a couple of other fixes.
-Harsha


>
>> > Signed-off-by: Sriharsha Basavapatna <
>> sriharsha.basavapatna at broadcom.com>
>> > ---
>> >   lib/netdev-offload-dpdk.c | 22 ----------------------
>> >   1 file changed, 22 deletions(-)
>> >
>> > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>> > index 4c652fd82..165fd1f47 100644
>> > --- a/lib/netdev-offload-dpdk.c
>> > +++ b/lib/netdev-offload-dpdk.c
>> > @@ -596,7 +596,6 @@ static int
>> >   parse_flow_match(struct flow_patterns *patterns,
>> >                    const struct match *match)
>> >   {
>> > -    uint8_t *next_proto_mask = NULL;
>> >       uint8_t proto = 0;
>> >
>> >       /* Eth */
>> > @@ -667,7 +666,6 @@ parse_flow_match(struct flow_patterns *patterns,
>> >           /* Save proto for L4 protocol setup. */
>> >           proto = spec->hdr.next_proto_id &
>> >                   mask->hdr.next_proto_id;
>> > -        next_proto_mask = &mask->hdr.next_proto_id;
>> >       }
>> >
>> >       if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  &&
>> > @@ -701,11 +699,6 @@ parse_flow_match(struct flow_patterns *patterns,
>> >           mask->hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) & 0xff;
>> >
>> >           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_TCP, spec,
>> mask);
>> > -
>> > -        /* proto == TCP and ITEM_TYPE_TCP, thus no need for proto
>> match. */
>> > -        if (next_proto_mask) {
>> > -            *next_proto_mask = 0;
>> > -        }
>> >       } else if (proto == IPPROTO_UDP) {
>> >           struct rte_flow_item_udp *spec, *mask;
>> >
>> > @@ -719,11 +712,6 @@ parse_flow_match(struct flow_patterns *patterns,
>> >           mask->hdr.dst_port = match->wc.masks.tp_dst;
>> >
>> >           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_UDP, spec,
>> mask);
>> > -
>> > -        /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto
>> match. */
>> > -        if (next_proto_mask) {
>> > -            *next_proto_mask = 0;
>> > -        }
>> >       } else if (proto == IPPROTO_SCTP) {
>> >           struct rte_flow_item_sctp *spec, *mask;
>> >
>> > @@ -737,11 +725,6 @@ parse_flow_match(struct flow_patterns *patterns,
>> >           mask->hdr.dst_port = match->wc.masks.tp_dst;
>> >
>> >           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_SCTP, spec,
>> mask);
>> > -
>> > -        /* proto == SCTP and ITEM_TYPE_SCTP, thus no need for proto
>> match. */
>> > -        if (next_proto_mask) {
>> > -            *next_proto_mask = 0;
>> > -        }
>> >       } else if (proto == IPPROTO_ICMP) {
>> >           struct rte_flow_item_icmp *spec, *mask;
>> >
>> > @@ -755,11 +738,6 @@ parse_flow_match(struct flow_patterns *patterns,
>> >           mask->hdr.icmp_code = (uint8_t) ntohs(match->wc.masks.tp_dst);
>> >
>> >           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ICMP, spec,
>> mask);
>> > -
>> > -        /* proto == ICMP and ITEM_TYPE_ICMP, thus no need for proto
>> match. */
>> > -        if (next_proto_mask) {
>> > -            *next_proto_mask = 0;
>> > -        }
>> >       }
>> >
>> >       add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
>>
>