Re: [v4,2/2] net/zxdh: add support flow director ops

[Ivan Malov <ivan.malov@arknetworks.am>]

[-- Attachment #1: Type: text/plain, Size: 6562 bytes --]

Hi,

On Tue, 12 Aug 2025, Junlong Wang wrote:

> Hi Maintainer,
> 
> > A general question: does the implementation somehow enforce any specific order
> > of actions? Say, check that ENCAP does not follow DROP? And does it need to
> > enforce, for example, a check that action DECAP is only used when the pattern
> > has matched on the very presence of an encap. header in the packet?
> 
>  1、The DROP action is only supported in ingress direction, And check that DECAP 
>  does not follow DROP.
>  2、It is based on matching the five-tuple of the packet to perform decap and encap
> 
> 
> 
> >> +void zxdh_flow_global_init(void)
> >> +{
> >> +    int i;
> >> +    for (i = 0; i < MAX_FLOW_COUNT_NUM; i++) {
> >> +        rte_spinlock_init(&flow_count_ref[i].count_lock);
> >> +        flow_count_ref[i].count_ref = 0;
> 
> > Just a question: does this absolutely need to be PMD-global and not per-device?
> 
>  Yes, currently, one card only supports 2048 fd rules, 

You say 'one card'. But isn't the driver capable of serving multiple cards
installed on the host and passed to DPDK? If yes, then does it make sense to
confine the total rule capacity of multiple cards to that of just one card?

Perhaps it's just my misunderstanding.

>  and there is no limit on the maximum usage of single pf/vf.
> 
> 
> 
> >> +    print_ether_addr("\nL2\t  dst=", &key->mac_dst, print_buf, buf_size, cur_len);
> >> +    print_ether_addr(" - src=", &key->mac_src, print_buf, buf_size, cur_len);
> >> +    MKDUMPSTR(print_buf, buf_size, *cur_len, " -eth type=0x%04x", key->ether_type);
> 
> > Just to make sure: is this in big-endian or little-endian? The original value of
> > EtherType supposedly comes from RTE flow item spec as a big-endian value.
> 
>  little-endian, HW supports little-endian.
>  We parsed from the RTE flow item spec, performed byte-order conversion,
>  and assigned it to the key.

Sorry - I might've overlooked that in the code. Did not see endianness
converstions in 'fd_flow_parse_pattern', for example. Or does 'data_bitwise' do
that? If yes, then perhaps add a comment to that helper to make it clearer.

> 
> 
> 
> >> +    count->bytes = (uint64_t)(rte_le_to_cpu_32(fstats.hit_bytes_hi)) << 32 |
> >> +                    rte_le_to_cpu_32(fstats.hit_bytes_lo);
> >> +    count->hits = (uint64_t)(rte_le_to_cpu_32(fstats.hit_pkts_hi)) << 32 |
> >> +                    rte_le_to_cpu_32(fstats.hit_pkts_lo);
> 
> > Shouldn't this also check for 'count->reset' (input field)? If the HW does not
> > support resetting the counter upon query, perhaps reject the query then?
> 
> > Also, this sets 'bytes' and 'hits', but not indicate the values are valid by
> > setting 'bytes_set' and 'hits_set'. Why? Please take a look at other PMDs.
> 
>  HW supports query by default, but we currently do not support determining 
>  whether to enable query based on count->reset.
> 
>  Currently, it is not supported to indicate that 'bytes' and 'hits' are valid
>  by setting 'bytes_set' and 'hits_set'.

Do you mean HW does not guarantee that 'hit_bytes_hi/lo' and 'hit_pkts_hi/lo'
are at all valid? That's odd. Perhaps the driver can indicate 'hits/bytes_set'
at least when the actual counters are non-zero?

I may be wrong, but it appears DPDK-based applications expect these 'set' fields
to be 'true' when reading the counters. For example, take a look at OvS [1].

[1] https://github.com/openvswitch/ovs/blob/00dcc546a1b9669b66a8985ceecd8266e33607d7/lib/netdev-offload-dpdk.c#L2562

> 
> 
> 
> 
> >> +        case RTE_FLOW_ITEM_TYPE_ETH:
> >> +            eth_spec = item->spec;
> >> +            eth_mask = item->mask;
> >> +            if (eth_spec && eth_mask) {
> >> +                key->mac_dst = eth_spec->dst;
> >> +                key->mac_src  = eth_spec->src;
> >> +                key_mask->mac_dst  = eth_mask->dst;
> >> +                key_mask->mac_src  = eth_mask->src;
> >> +
> >> +                if (eth_mask->type == 0xffff) {
> >> +                    key->ether_type = eth_spec->type;
> >> +                    key_mask->ether_type = eth_mask->type;
> >> +                }
> >> +            }
> 
> > 1) If both 'spec' and 'mask' are 'NULL', shouldn't the code set some broader
> > match on the sole presence of a Ethernet header?
> > 2) If 'mask' is 'NULL' and 'spec' is not (or vice versa), isn't this an error?
> 
> >> +            break;
> >> +        case RTE_FLOW_ITEM_TYPE_VLAN:
> >> +            vlan_spec = item->spec;
> >> +            vlan_mask = item->mask;
> >> +            if (vlan_spec && vlan_mask) {
> >> +                key->vlan_tci  = vlan_spec->tci;
> >> +                key_mask->vlan_tci = vlan_mask->tci;
> >> +            }
> 
> > Similar questions here. For example, the user having provided the item VLAN
> > but having omitted both 'spec' and 'mask' can be translated into setting the
> > VLAN EtherType in 'key->ether_type', with an appropriate check of whether this
> > key has already been set to an overlapping value by the previous (ETH) item.
> 
>  In my opinion, The values of 'spec' and 'mask' will not be null.
>  If the user does not set 'spec' and 'mask', the passed-in values will be set to 00 and ff by default,
>  which is ensured by the upper-layer interface of dpdk.

Wait a minute.. does this mean that if the user passes, say, ETH / IPV4 / UDP
pattern and the 'spec' and 'mask' of ETH are both 'null', the actual key/mask
values in the HW rule will be, say, for the EtherType, '0000' and 'ffff'?
Is this correct? And which part of DPDK interface envisages that?

Again, may be it's just my misunderstanding.

Thank you.

> 
> 
>  Other issues will de modified, and will be resubbmitted.
> 
>