Hi Ilya, Thanks for review comments.
> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Wednesday, March 30, 2022 10:46 PM > To: Amber, Kumar <[email protected]>; [email protected] > Cc: [email protected]; Ferriter, Cian <[email protected]>; Stokes, Ian > <[email protected]>; [email protected]; [email protected]; Van > Haaren, Harry <[email protected]> > Subject: Re: [PATCH v1] dpcls: Add the dpcls subtable lookup function in flow > dump. > > On 3/21/22 18:06, Kumar Amber wrote: > > The patch adds the subtable lookup name to the existing dp-extra-info > > mentioned below; > > Thanks for the patch! It seems even smaller than I expected. > Some comments inline. > > > > > dp-extra-info:miniflow_bits(18,4), lookup(generic) > > dp-extra-info:miniflow_bits(9,4), lookup(avx512_gather) > > There should be no extra space in the output, since both the bits and lookup > method are parts of dp-extra-info. > Otherwise, it's harder to parse the flow dump. > Fixed in V2. > > > > Suggested-by: Ilya Maximets <[email protected]> > > Signed-off-by: Kumar Amber <[email protected]> > > Signed-off-by: Harry van Haaren <[email protected]> > > Co-authored-by: Harry van Haaren <[email protected]> > > --- > > lib/dpif-netdev-lookup.c | 10 ++++++---- > > lib/dpif-netdev-lookup.h | 8 +++++++- > > lib/dpif-netdev-private-dpcls.h | 3 +++ > > lib/dpif-netdev.c | 15 ++++++++++++--- > > 4 files changed, 28 insertions(+), 8 deletions(-) > > > > diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c index > > bd0a99abe..accf5a21b 100644 > > --- a/lib/dpif-netdev-lookup.c > > +++ b/lib/dpif-netdev-lookup.c > > @@ -93,11 +93,11 @@ dpcls_subtable_set_prio(const char *name, uint8_t > > priority) } > > > > dpcls_subtable_lookup_func > > -dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t > > u1_bit_count) > > +dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t > u1_bit_count, > > + const char **subtable_name_out) > > For this function, the argument can be just called 'name', as there are no > other names here. > Fixed in V2, Makes sense. > > { > > /* Iter over each subtable impl, and get highest priority one. */ > > int32_t prio = -1; > > - const char *name = NULL; > > dpcls_subtable_lookup_func best_func = NULL; > > > > for (int i = 0; i < ARRAY_SIZE(subtable_lookups); i++) { @@ > > -109,13 +109,15 @@ dpcls_subtable_get_best_impl(uint32_t u0_bit_count, > uint32_t u1_bit_count) > > if (probed_func) { > > best_func = probed_func; > > prio = probed_prio; > > - name = subtable_lookups[i].name; > > + if (subtable_name_out) { > > + *subtable_name_out = subtable_lookups[i].name; > > + } > > } > > } > > } > > > > VLOG_DBG("Subtable lookup function '%s' with units (%d,%d), priority > %d\n", > > - name, u0_bit_count, u1_bit_count, prio); > > + *subtable_name_out, u0_bit_count, u1_bit_count, prio); > > > > /* Programming error - we must always return a valid func ptr. */ > > ovs_assert(best_func != NULL); > > diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h index > > 59f51faa0..f39d622a9 100644 > > --- a/lib/dpif-netdev-lookup.h > > +++ b/lib/dpif-netdev-lookup.h > > @@ -66,8 +66,14 @@ struct dpcls_subtable_lookup_info_t { > > > > int32_t dpcls_subtable_set_prio(const char *name, uint8_t priority); > > > > +/* Retrieve the best implementation for dpcls subtable. > > + * subtable_name_out parameter is used to fetch the name of dpcls > > +subtable > > + * selected by the function. > > + * The function can also be called with subtable_name_out as NULL. > > + */ > > dpcls_subtable_lookup_func > > -dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t > > u1_bit_count); > > +dpcls_subtable_get_best_impl(uint32_t u0_bit_count, uint32_t > u1_bit_count, > > + const char **subtable_name_out); > > > > /* Retrieve the array of lookup implementations for iteration. > > * On error, returns a negative number. > > diff --git a/lib/dpif-netdev-private-dpcls.h > > b/lib/dpif-netdev-private-dpcls.h index 0d5da73c7..76ca5b29a 100644 > > --- a/lib/dpif-netdev-private-dpcls.h > > +++ b/lib/dpif-netdev-private-dpcls.h > > @@ -93,6 +93,9 @@ struct dpcls_subtable { > > > > struct netdev_flow_key mask; /* Wildcards for fields (const). */ > > /* 'mask' must be the last field, additional space is allocated > > here. */ > > + > > + /* Holds the name of the DPCLS implementation slected. */ > > + const char *subtable_name; > > 'subtable_name' doesn't sound right. It's not a name of a subtable after all. > Maybe 'lookup_impl_name' or 'subtable_lookup_name' ? > 'subtable_lookup_impl_name' might be the most correct, but it gets too long, > IMO. > Yes agreed, 'subtable_lookup_name' makes more sense. > > }; > > > > /* Iterate through netdev_flow_key TNL u64 values specified by > > 'FLOWMAP'. */ diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index > > 720818e30..f667b0b67 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -624,6 +624,9 @@ inline struct dpcls * > > dp_netdev_pmd_lookup_dpcls(struct dp_netdev_pmd_thread *pmd, > > odp_port_t in_port); > > > > +static inline struct dpcls_subtable * dpcls_find_subtable(struct > > +dpcls *cls, const struct netdev_flow_key *mask); > > + > > static void dp_netdev_request_reconfigure(struct dp_netdev *dp); > > static inline bool pmd_perf_metrics_enabled(const struct > > dp_netdev_pmd_thread *pmd); @@ -4115,6 +4118,11 @@ > > dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd, > > count_1bits(flow->cr.mask->mf.map.bits[unit])); > > } > > ds_put_char(&extra_info, ')'); > > + > > + struct dpcls_subtable *subtable = dpcls_find_subtable(cls, &mask); > > + ds_put_format(&extra_info, ", lookup(%s) ", > > Both spaces are redundant here. > Fixed. > > + subtable->subtable_name); > > + > > flow->dp_extra_info = ds_steal_cstr(&extra_info); > > ds_destroy(&extra_info); > > > > @@ -9748,8 +9756,8 @@ dpcls_create_subtable(struct dpcls *cls, const > struct netdev_flow_key *mask) > > * by the PMD thread. > > */ > > atomic_init(&subtable->lookup_func, > > - dpcls_subtable_get_best_impl(unit0, unit1)); > > - > > + dpcls_subtable_get_best_impl(unit0, unit1, > > + &subtable->subtable_name)); > > cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask- > >hash); > > /* Add the new subtable at the end of the pvector (with no hits yet) */ > > pvector_insert(&cls->subtables, subtable, 0); @@ -9797,7 +9805,8 > > @@ dpcls_subtable_lookup_reprobe(struct dpcls *cls) > > /* Set the subtable lookup function atomically to avoid garbage > > data > > * being read by the PMD thread. */ > > atomic_store_relaxed(&subtable->lookup_func, > > - dpcls_subtable_get_best_impl(u0_bits, u1_bits)); > > + dpcls_subtable_get_best_impl(u0_bits, u1_bits, > > + > > + &subtable->subtable_name)); > > This will change the value stored in the structure, but that will not change > the > value of the 'flow->dp_extra_info' that will still report the name of the old > implementation during the flow dump. > dp_extra_info generated only once when the flow is created. So, you need > to re-generate it here with the new name. That's why I was talking about > protecting that pointer with RCU. > > You should be able to write a test for that scenario. > Thanks Ilya, realized in after sending the patch using lib "ovs-rcu.h" trying to fix Pointer updating mentioned above. Regards AMber > > subtables_changed += (old_func != subtable->lookup_func); > > } > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
