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. > > 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. > { > /* 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. > }; > > /* 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. > + 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. > subtables_changed += (old_func != subtable->lookup_func); > } > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
