[ovs-dev] [v4 02/12] dpif-netdev: Add auto validation function for miniflow extract

Amber, Kumar kumar.amber at intel.com
Tue Jun 29 15:20:36 UTC 2021


Hi Flavio,

Replies inline.

> >
> > 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?
> 

Yes True sorry for confusion so we have refactored the code a bit to use Atomic set and get along with the API 
Wherever applicable since here on any failure we would want to fall back to Scalar we would not need the API
To find default implementation.
> >
> > + 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.

Sorry for confusion I have fixed all the VLOGS with this convention.
> 
> 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]; l3_ofs = good_l3_ofs[i]; l4_ofs
> > + packet->= good_l4_ofs[i]; 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
> > dev at openvswitch.org<mailto:dev at openvswitch.org>
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> --
> fbl


More information about the dev mailing list