Hi Flavio, Thanks Again replies are inlined.
> > /* Implementations of available extract options. */ static struct > > dpif_miniflow_extract_impl mfex_impls[] = { > > + { > > + .probe = NULL, > > + .extract_func = dpif_miniflow_extract_autovalidator, > > + .name = "autovalidator", > > + }, > > Please define a enum for each entry. Also document that autovalidator is > required to be the first and suggest to see the comment explaining more on > MFEX_IMPL_START_IDX. > Fixed in upcoming v5. Using ENUMS directly. > > { > > .probe = NULL, > > .extract_func = NULL, > > @@ -84,3 +89,139 @@ dpif_miniflow_extract_info_get(struct > dpif_miniflow_extract_impl **out_ptr) > > *out_ptr = mfex_impls; > > return ARRAY_SIZE(mfex_impls); > > } > > + > > +uint32_t > > +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *packets, > > + struct netdev_flow_key *keys, > > + uint32_t keys_size, odp_port_t in_port, > > + void *pmd_handle) { > > + const size_t cnt = dp_packet_batch_size(packets); > > + uint16_t good_l2_5_ofs[NETDEV_MAX_BURST]; > > + uint16_t good_l3_ofs[NETDEV_MAX_BURST]; > > + uint16_t good_l4_ofs[NETDEV_MAX_BURST]; > > + uint16_t good_l2_pad_size[NETDEV_MAX_BURST]; > > + struct dp_packet *packet; > > + struct dp_netdev_pmd_thread *pmd = pmd_handle; > > + struct dpif_miniflow_extract_impl *miniflow_funcs; > > + > > + int32_t mfunc_count = > dpif_miniflow_extract_info_get(&miniflow_funcs); > > + if (mfunc_count < 0) { > > + pmd->miniflow_extract_opt = NULL; > > + VLOG_ERR("failed to get miniflow extract function > > + implementations\n"); > > No need for terminating with \n here and other calls to VLOG_*(). > Fixed in upcoming v5. > > + return 0; > > + } > > > + ovs_assert(keys_size >= cnt); > > + struct netdev_flow_key test_keys[NETDEV_MAX_BURST]; > > + > > + /* Run scalar miniflow_extract to get default result. */ > > + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) { > > + pkt_metadata_init(&packet->md, in_port); > > + miniflow_extract(packet, &keys[i].mf); > > + > > + /* Store known good metadata to compare with optimized > metadata. */ > > + good_l2_5_ofs[i] = packet->l2_5_ofs; > > + good_l3_ofs[i] = packet->l3_ofs; > > + good_l4_ofs[i] = packet->l4_ofs; > > + good_l2_pad_size[i] = packet->l2_pad_size; > > + } > > + > > + /* Iterate through each version of miniflow implementations. */ > > + for (int j = MFEX_IMPL_START_IDX; j < ARRAY_SIZE(mfex_impls); j++) { > > + if (!mfex_impls[j].available) { > > + continue; > > + } > > + > > + /* Reset keys and offsets before each implementation. */ > > + memset(test_keys, 0, keys_size * sizeof(struct netdev_flow_key)); > > + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) { > > + dp_packet_reset_offsets(packet); > > + } > > + /* Call optimized miniflow for each batch of packet. */ > > + uint32_t hit_mask = mfex_impls[j].extract_func(packets, test_keys, > > + keys_size, in_port, > > + pmd_handle); > > + > > + /* Do a miniflow compare for bits, blocks and offsets for all the > > + * classified packets in the hitmask marked by set bits. */ > > + while (hit_mask) { > > + /* Index for the set bit. */ > > + uint32_t i = __builtin_ctz(hit_mask); > > + /* Set the index in hitmask to Zero. */ > > + hit_mask &= (hit_mask - 1); > > + > > + uint32_t failed = 0; > > + > > + /* Check miniflow bits are equal. */ > > + if ((keys[i].mf.map.bits[0] != test_keys[i].mf.map.bits[0]) || > > + (keys[i].mf.map.bits[1] != test_keys[i].mf.map.bits[1])) { > > + VLOG_ERR("Good 0x%llx 0x%llx\tTest 0x%llx 0x%llx\n", > > + keys[i].mf.map.bits[0], keys[i].mf.map.bits[1], > > + test_keys[i].mf.map.bits[0], > > + test_keys[i].mf.map.bits[1]); > > + failed = 1; > > + } > > + > > + if (!miniflow_equal(&keys[i].mf, &test_keys[i].mf)) { > > + uint32_t block_cnt = miniflow_n_values(&keys[i].mf); > > + VLOG_ERR("Autovalidation blocks failed for %s pkt %d", > > + mfex_impls[j].name, i); > > + VLOG_ERR(" Good hexdump:\n"); > > + uint64_t *good_block_ptr = (uint64_t *)&keys[i].buf; > > + uint64_t *test_block_ptr = (uint64_t *)&test_keys[i].buf; > > + for (uint32_t b = 0; b < block_cnt; b++) { > > + VLOG_ERR(" %"PRIx64"\n", good_block_ptr[b]); > > + } > > + VLOG_ERR(" Test hexdump:\n"); > > + for (uint32_t b = 0; b < block_cnt; b++) { > > + VLOG_ERR(" %"PRIx64"\n", test_block_ptr[b]); > > + } > > + failed = 1; > > + } > > + > > What do you think of doing this instead: > packet = packets->packets[i]; Does look nice taken into v5. > if ((packet->l2_pad_size != good_l2_pad_size[i]) || > ... > > > + if ((packets->packets[i]->l2_pad_size != > > + good_l2_pad_size[i]) || > > > + (packets->packets[i]->l2_5_ofs != good_l2_5_ofs[i]) || > > + (packets->packets[i]->l3_ofs != good_l3_ofs[i]) || > > + (packets->packets[i]->l4_ofs != good_l4_ofs[i])) { > > + VLOG_ERR("Autovalidation packet offsets failed for %s pkt > > %d", > > + mfex_impls[j].name, i); > > + VLOG_ERR(" Good offsets: l2_pad_size %u, l2_5_ofs : %u" > > + " l3_ofs %u, l4_ofs %u\n", > > + good_l2_pad_size[i], good_l2_5_ofs[i], > > + good_l3_ofs[i], good_l4_ofs[i]); > > + VLOG_ERR(" Test offsets: l2_pad_size %u, l2_5_ofs : %u" > > + " l3_ofs %u, l4_ofs %u\n", > > + packets->packets[i]->l2_pad_size, > > + packets->packets[i]->l2_5_ofs, > > + packets->packets[i]->l3_ofs, > > + packets->packets[i]->l4_ofs); > > + failed = 1; > > + } > > + > > + if (failed) { > > + /* Having dumped the debug info, disable autovalidator. */ > > + VLOG_ERR("Autovalidation failed in %s pkt %d, > > disabling.\n", > > + mfex_impls[j].name, i); > > + /* Halt OVS here on debug builds. */ > > + ovs_assert(0); > > + pmd->miniflow_extract_opt = NULL; > > + break; > > + } > > + } > > + } > > + > > + /* preserve packet correctness by storing back the good offsets in > > + * packets back. */ > > + DP_PACKET_BATCH_FOR_EACH (i, packet, packets) { > > + packet->l2_5_ofs = good_l2_5_ofs[i]; > > + packet->l3_ofs = good_l3_ofs[i]; > > + packet->l4_ofs = good_l4_ofs[i]; > > + packet->l2_pad_size = good_l2_pad_size[i]; > > + } > > + > > + /* Returning zero implies no packets were hit by autovalidation. This > > + * simplifies unit-tests as changing > > --enable-mfex-default-autovalidator > > + * would pass/fail. By always returning zero, autovalidator is a little > > + * slower, but we gain consistency in testing. > > + */ > > + return 0; > > +} > > diff --git a/lib/dpif-netdev-private-extract.h > > b/lib/dpif-netdev-private-extract.h > > index b7b0b2be4..455a7b590 100644 > > --- a/lib/dpif-netdev-private-extract.h > > +++ b/lib/dpif-netdev-private-extract.h > > @@ -24,6 +24,11 @@ > > /* Max size of dpif_miniflow_extract_impl array. */ #define > > MFEX_IMPLS_MAX_SIZE (16) > > > > +/* Skip the autovalidator study and null when iterating all available > > + * miniflow implementations. > > + */ > > +#define MFEX_IMPL_START_IDX (1) > > + > > As pointed above, this should come from an enum. Fixed in upcoming v5. > > > > /* Forward declarations. */ > > struct dp_packet; > > struct miniflow; > > @@ -90,5 +95,15 @@ dpif_miniflow_extract_init(void); int32_t > > dpif_miniflow_extract_info_get(struct dpif_miniflow_extract_impl > > **out_ptr); > > > > +/* Retrieve the hitmask of the batch of pakcets which is obtained by > > +comparing > > + * different miniflow implementations with linear miniflow extract. > > + * On error, returns a zero. > > + * On success, returns the number of packets in the batch compared. > > + */ > > +uint32_t > > +dpif_miniflow_extract_autovalidator(struct dp_packet_batch *batch, > > + struct netdev_flow_key *keys, > > + uint32_t keys_size, odp_port_t in_port, > > + void *pmd_handle); > > > > #endif /* DPIF_NETDEV_AVX512_EXTRACT */ diff --git > > a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 567ebd952..4f4ab2790 > > 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -1181,8 +1181,8 @@ dpif_miniflow_extract_impl_set(struct > unixctl_conn *conn, int argc, > > struct ds reply = DS_EMPTY_INITIALIZER; > > ds_put_format(&reply, "Miniflow implementation set to %s.\n", > mfex_name); > > const char *reply_str = ds_cstr(&reply); > > - unixctl_command_reply(conn, reply_str); > > VLOG_INFO("%s", reply_str); > > + unixctl_command_reply(conn, reply_str); > > ds_destroy(&reply); > > } > > > > -- > > 2.25.1 > > > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- > fbl _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev