Hi,
On Tue, Jun 29, 2021 at 12:27:57PM +0000, Amber, Kumar wrote: > Hi Eelco, > > Thanks Again for reviews , Pls find my replies inline. > > From: Eelco Chaudron <echau...@redhat.com> > Sent: Tuesday, June 29, 2021 5:14 PM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; Amber, Kumar > <kumar.am...@intel.com> > Cc: d...@openvswitch.org; i.maxim...@ovn.org; Stokes, Ian > <ian.sto...@intel.com>; Flavio Leitner <f...@sysclose.org> > Subject: Re: [ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function > for miniflow extract > > > On 17 Jun 2021, at 18:27, Kumar Amber wrote: > > This patch introduced the auto-validation function which > allows users to compare the batch of packets obtained from > different miniflow implementations against the linear > miniflow extract and return a hitmask. > > The autovaidator function can be triggered at runtime using the > following command: > > $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator > > Signed-off-by: Kumar Amber > <kumar.am...@intel.com<mailto:kumar.am...@intel.com>> > Co-authored-by: Harry van Haaren > <harry.van.haa...@intel.com<mailto:harry.van.haa...@intel.com>> > Signed-off-by: Harry van Haaren > <harry.van.haa...@intel.com<mailto:harry.van.haa...@intel.com>> > --- > lib/dpif-netdev-private-extract.c | 141 ++++++++++++++++++++++++++++++ > lib/dpif-netdev-private-extract.h | 15 ++++ > lib/dpif-netdev.c | 2 +- > 3 files changed, 157 insertions(+), 1 deletion(-) > > diff --git a/lib/dpif-netdev-private-extract.c > b/lib/dpif-netdev-private-extract.c > index fcc56ef26..0741c19f9 100644 > --- a/lib/dpif-netdev-private-extract.c > +++ b/lib/dpif-netdev-private-extract.c > @@ -32,6 +32,11 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev_extract); > > /* Implementations of available extract options. */ > static struct dpif_miniflow_extract_impl mfex_impls[] = { > + { > + .probe = NULL, > + .extract_func = dpif_miniflow_extract_autovalidator, > + .name = "autovalidator", > + }, > { > .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) { > > In theory 0 could not be returned, but just to cover the corner case can we > change this to include zero. > > The code has been adapted as per Flavio comments so will not be a concern. > > + pmd->miniflow_extract_opt = NULL; > > Guess the above needs to be atomic. > > Removed based on Flavio comments. I asked to initialize that using an API and Eelco is asking to set it atomically. The requests are complementary, right? > > + VLOG_ERR("failed to get miniflow extract function implementations\n"); > > Capital F to be in sync with your other error messages? > > Removed based on Flavio comments. Not sure if I got this. I mentioned that the '\n' is not needed at the end of all VLOG_* calls. Eelco is asking to start with capital 'F'. So the requests are complementary, unless with the refactor the message went away. Just make sure to follow the logging style convention in OVS. fbl > > + return 0; > + } > + ovs_assert(keys_size >= cnt); > > I don’t think we should assert here. Just return an error like above, so in > production, we get notified, and this implementation gets disabled. > > Actually we do else one would most likely be overwriting the assigned array > space for keys and will hit a Seg fault at some point. > > And hence we would like to know at the compile time if this is the case. > > + 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; > + } > + > + 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) { > > Why stop now!? I think we should run all implementations, as others might > need fixing too! > > We had the same model as above by you but with so many debug messages > flooding made it impossible for us to > > Know the root cause and ovs_assert(0); have been removes so will no longer > cause a crash but will simply disable the autovalidator > > + /* 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) > + > /* 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); > > Forgot to comment on this in my previous patch: > > /* Function pointer prototype to be implemented in the optimized miniflow > > * extract code. > * returns the hitmask of the processed packets on success. > * returns zero on failure. > > */ > typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch *batch, > struct netdev_flow_key *keys, > uint32_t keys_size, > odp_port_t in_port, > void *pmd_handle); > > Maybe we could add some description of what the input parameters mean, are > expected? For example that key_size need to be at least the size of the > batch? If this is the case, do we need it, and can we just assume keys should > be at least batch size long? > > Yes put a comment in there about key size in the description in v5. > > But as per convention whenever we pass an array we should also put the size > of the array and this also complies with various > > Security checkers and also prevents potential exploitations. > > +/* 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<mailto:d...@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > 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
