On 29 Jun 2021, at 18:15, Amber, Kumar wrote:
> Hi Eelco , > > Sorry the formatting seems broken on this email thread. > Replies are inlined . Thanks, looking forward to the v5 (hopefully once I finished this version of the series. I’m currently at patch 10 ;) > From: Eelco Chaudron <echau...@redhat.com> > Sent: Tuesday, June 29, 2021 7:36 PM > To: Amber, Kumar <kumar.am...@intel.com> > Cc: Van Haaren, Harry <harry.van.haa...@intel.com>; 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 > > > Not sure how you replied, but it’s hard to see which comments are mine, and > which are yours. > > On 29 Jun 2021, at 14:27, Amber, Kumar wrote: > > Hi Eelco, > > Thanks Again for reviews , Pls find my replies inline. > > From: Eelco Chaudron <echau...@redhat.com<mailto:echau...@redhat.com>> > Sent: Tuesday, June 29, 2021 5:14 PM > To: Van Haaren, Harry > <harry.van.haa...@intel.com<mailto:harry.van.haa...@intel.com>> ; Amber, > Kumar <kumar.am...@intel.com<mailto:kumar.am...@intel.com>> > Cc: d...@openvswitch.org<mailto:d...@openvswitch.org> ; > i.maxim...@ovn.org<mailto:i.maxim...@ovn.org> ; Stokes, Ian > <ian.sto...@intel.com<mailto:ian.sto...@intel.com>> ; Flavio Leitner > <f...@sysclose.org<mailto: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. > > + 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. > > + 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. > > But this is not a compile time check, it will crash OVS. You could just do > this: > > if (keys_size < cnt) { > pmd->miniflow_extract_opt = NULL; > VLOG_ERR(“Invalid key size supplied etc. etc.\n”); > return 0; > } > > Or you could process up to key_size packets > > Reply: sure I have taken the first approach in v5 as it safe and avoid any > risk of Seg fault in V5. > > + 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 > > I still would opt for getting all in the log. What is worse than getting a > customer to crash (LOG) one failure, and with the provided fix, it will > crash/log the next one :( > And if the logs were not clear enough, maybe they need some rewriting? > > Reply: As pointed out by Flavio in patch 07/12 , I guess we can do the > following here as: > > - Remove the ovs_assert() > > - VLOG_ERR() any mismatches > > - Continue processing rest of burst with MFEX Autovalidator > > On failures, we disable autovalidator *after* the burst, to avoid spamming > output and filling logs. > > + /* 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. > > Ack > > +/* 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
