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, 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. >> + 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'. >> + 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. Other issues will de modified, and will be resubbmitted.