[ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for miniflow extract
Amber, Kumar
kumar.amber at intel.com
Mon Jun 28 15:04:09 UTC 2021
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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
More information about the dev
mailing list