Hi Kumar, Thanks for fixing the previously mentioned issues, however, we still need to finish the API discussion. I copied over the discussion from v2 below.
//Eelco On 28 Oct 2021, at 11:50, Kumar Amber wrote: > Modified the dplcs info-get command output to include > the count for different dpcls implementations. > > $ovs-appctl dpif-netdev/subtable-lookup-prio-get > > Available dpcls implementations: > autovalidator (Use count: 1, Priority: 5) > generic (Use count: 0, Priority: 1) > avx512_gather (Use count: 0, Priority: 3) > > Test case to verify changes: > 1021: PMD - dpcls configuration ok > > Signed-off-by: Kumar Amber <[email protected]> > Signed-off-by: Harry van Haaren <[email protected]> > Co-authored-by: Harry van Haaren <[email protected]> > > --- > v3: > - Fix comments on the patch. > - Function API remains same, see discussion on OVS ML here: > "https://mail.openvswitch.org/pipermail/ovs-dev/2021-October/388737.html"; > v2: > - Dependency merged rebased to master. > > --- > --- > Documentation/topics/dpdk/bridge.rst | 16 +++---- > lib/dpif-netdev-lookup.c | 72 +++++++++++++++++++++++----- > lib/dpif-netdev-lookup.h | 19 +++++++- > lib/dpif-netdev.c | 31 +++++------- > tests/pmd.at | 16 +++---- > 5 files changed, 106 insertions(+), 48 deletions(-) > > diff --git a/Documentation/topics/dpdk/bridge.rst > b/Documentation/topics/dpdk/bridge.rst > index f645b9ade..63a54da1c 100644 > --- a/Documentation/topics/dpdk/bridge.rst > +++ b/Documentation/topics/dpdk/bridge.rst > @@ -156,10 +156,10 @@ OVS provides multiple implementations of dpcls. The > following command enables > the user to check what implementations are available in a running instance :: > > $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > - Available lookup functions (priority : name) > - 0 : autovalidator > - 1 : generic > - 0 : avx512_gather > + Available dpcls implementations: > + autovalidator (Use count: 1, Priority: 5) > + generic (Use count: 0, Priority: 1) > + avx512_gather (Use count: 0, Priority: 3) > > To set the priority of a lookup function, run the ``prio-set`` command :: > > @@ -172,10 +172,10 @@ function due to the command being run. To verify the > prioritization, re-run the > get command, note the updated priority of the ``avx512_gather`` function :: > > $ ovs-appctl dpif-netdev/subtable-lookup-prio-get > - Available lookup functions (priority : name) > - 0 : autovalidator > - 1 : generic > - 5 : avx512_gather > + Available dpcls implementations: > + autovalidator (Use count: 0, Priority: 0) > + generic (Use count: 0, Priority: 0) > + avx512_gather (Use count: 1, Priority: 5) > > If two lookup functions have the same priority, the first one in the list is > chosen, and the 2nd occurance of that priority is not used. Put in logical > diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c > index bd0a99abe..8a95c84e1 100644 > --- a/lib/dpif-netdev-lookup.c > +++ b/lib/dpif-netdev-lookup.c > @@ -36,18 +36,21 @@ static struct dpcls_subtable_lookup_info_t > subtable_lookups[] = { > { .prio = 0, > #endif > .probe = dpcls_subtable_autovalidator_probe, > - .name = "autovalidator", }, > + .name = "autovalidator", > + .usage_cnt.count = 0,}, Use the ATOMIC_COUNT_INIT() macro, and also add a space after the comma: .usage_cnt = ATOMIC_COUNT_INIT(0), }, > > /* The default scalar C code implementation. */ > { .prio = 1, > .probe = dpcls_subtable_generic_probe, > - .name = "generic", }, > + .name = "generic", > + .usage_cnt.count = 0,}, Same as above > > #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__) > /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */ > { .prio = 0, > .probe = dpcls_subtable_avx512_gather_probe, > - .name = "avx512_gather", }, > + .name = "avx512_gather", > + .usage_cnt.count = 0,}, Same as above > #else > /* Disabling AVX512 at compile time, as compile time requirements not > met. > * This could be due to a number of reasons: > @@ -93,25 +96,44 @@ 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, > + dpcls_subtable_lookup_func old_func, > + bool will_use_result) > { > /* Iter over each subtable impl, and get highest priority one. */ > int32_t prio = -1; > + uint32_t i; In this code you use a lot of (u)int32_t, however in OVS for this kind of use cases please stick to just int, and if really needed unsigned int. > const char *name = NULL; > + uint32_t best_idx = 0; See above, just use int here. > dpcls_subtable_lookup_func best_func = NULL; > > - for (int i = 0; i < ARRAY_SIZE(subtable_lookups); i++) { > + for (i = 0; i < ARRAY_SIZE(subtable_lookups); i++) { Think we should just keep the (int i) definition as is here. > int32_t probed_prio = subtable_lookups[i].prio; > + dpcls_subtable_lookup_func probed_func; > + > + probed_func = subtable_lookups[i].probe(u0_bit_count, > + u1_bit_count); > + if (!probed_func) { > + continue; > + } > + > + /* Better candidate - track this to return it later. */ > if (probed_prio > prio) { > - dpcls_subtable_lookup_func probed_func; > - probed_func = subtable_lookups[i].probe(u0_bit_count, > - u1_bit_count); > - if (probed_func) { > - best_func = probed_func; > - prio = probed_prio; > - name = subtable_lookups[i].name; > - } > + best_func = probed_func; > + best_idx = i; > + prio = probed_prio; > + name = subtable_lookups[i].name; > } > + > + /* Statistics keeping, reduce old func usage count. */ > + if (probed_func == old_func) { > + atomic_count_dec(&subtable_lookups[i].usage_cnt); > + } > + } > + > + /* Update stats for usage. */ > + if (will_use_result) { > + atomic_count_inc(&subtable_lookups[best_idx].usage_cnt); > } > > VLOG_DBG("Subtable lookup function '%s' with units (%d,%d), priority > %d\n", > @@ -122,3 +144,27 @@ dpcls_subtable_get_best_impl(uint32_t u0_bit_count, > uint32_t u1_bit_count) > > return best_func; > } > + > +void > +dp_dpcls_impl_print_stats(struct ds *reply) > +{ > + struct dpcls_subtable_lookup_info_t *lookup_funcs = NULL; > + int32_t count = dpcls_subtable_lookup_info_get(&lookup_funcs); > + > + /* Add all DPCLS functions to reply string. */ > + ds_put_cstr(reply, "Available dpcls implementations:\n"); > + > + for (int i = 0; i < count; i++) { > + ds_put_format(reply, " %s (Use count: %d, Priority: %d", > + lookup_funcs[i].name, > + atomic_count_get(&lookup_funcs[i].usage_cnt), > + lookup_funcs[i].prio); > + > + if (ds_last(reply) == ' ') { > + ds_put_cstr(reply, "none"); > + } > + > + ds_put_cstr(reply, ")\n"); > + } > + > +} > diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h > index 59f51faa0..c0dee4ff0 100644 > --- a/lib/dpif-netdev-lookup.h > +++ b/lib/dpif-netdev-lookup.h > @@ -20,6 +20,7 @@ > #include <config.h> > #include "dpif-netdev.h" > #include "dpif-netdev-private-dpcls.h" > +#include "dpif-netdev-private-thread.h" > > /* Function to perform a probe for the subtable bit fingerprint. > * Returns NULL if not valid, or a valid function pointer to call for this > @@ -62,12 +63,24 @@ struct dpcls_subtable_lookup_info_t { > > /* Human readable name, used in setting subtable priority commands */ > const char *name; > + > + /* Counter which holds the usage count of each implementations. */ > + atomic_count usage_cnt; > }; > > int32_t dpcls_subtable_set_prio(const char *name, uint8_t priority); > > +/* Lookup the best subtable lookup implementation for the given u0,u1 count. > + * When replacing an existing lookup func, the old_func pointer is provied > + * and statistics will be tracked accordingly and will_use parameter would > + * be set to 1 if the result of the function is going to be used for a > + * subtable lookup else when set to 0, the statistics will refect that the > + * result is not being used. > + */ > 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, > + dpcls_subtable_lookup_func old_func, > + bool will_use); > First some cut/pastes from the V2 discussion which was not completed: EC> Guess you overload this function to do a counter update, which I do not like (see comments below). HH> Yes - correct. I'd prefer keep separate APIs too, but in reality the implementation to increment a counter HH> would require a search through all subtables (which we've already just done). So iterating all dpcls implementations HH> a second time, doing a brute-force search in order to increment a variable seemed too much effort, as well as HH> maintaining 2x implementations. HH> The goal of adding the "will use result" parameter (overloading API) was to reduce code maintenance effort... --- EC> First, the 0, should probably be a false bool. But in general, I do not like this API EC> overload. Why would you call a dpcls_subtable_get_best_impl() to reset a EC> counter? Can we not add a new API in form or EC> dpcls_update_impl_use_count(struct dpcls_subtable *ubtable, bool increase), or EC> something else thats is more clear? HH> As per above, the "dpcls_update_impl_use_count()" would require a brute-force search through the tables again. HH> By incorporating the "increment" in the API, we can just iterate the implementations once. HH> I think it is worth the complexity to avoid re-iterating implementations a 2nd time. --- EC> If dpcls_subtable_get_best_impl() would always increase the use count, no EC> changes should be needed here. HH> I see where you're going with this approach - see above comments. HH> Would documenting the reasoning clearly as to why the API is overloaded, and keeping HH> implementation (1 function, overloeaded with will_use_result) the same as is today be acceptable? --- I think we should keep the API as clean as possible, and adding flags for all kinds of exceptions is not the way forward. This will result in a mess. I’m sure you guys can come up with a nice solution. Without giving this much thought, you could maybe have dpcls_subtable_get_best_impl() return a pointer to dpcls_subtable_lookup_info_t and store/use that in dpcls_subtable, and have dpcls_update_impl_use_count() use a pointer to dpcls_subtable->dpcls_subtable_lookup_info_t? > /* Retrieve the array of lookup implementations for iteration. > * On error, returns a negative number. > @@ -76,4 +89,8 @@ dpcls_subtable_get_best_impl(uint32_t u0_bit_count, > uint32_t u1_bit_count); > int32_t > dpcls_subtable_lookup_info_get(struct dpcls_subtable_lookup_info_t > **out_ptr); > > +/* Prints dpcls subtables in use for different implementations. */ > +void > +dp_dpcls_impl_print_stats(struct ds *reply); > + > #endif /* dpif-netdev-lookup.h */ > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index b078c2da5..194e6914f 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -911,21 +911,9 @@ dpif_netdev_subtable_lookup_get(struct unixctl_conn > *conn, int argc OVS_UNUSED, > const char *argv[] OVS_UNUSED, > void *aux OVS_UNUSED) > { > - /* Get a list of all lookup functions. */ > - struct dpcls_subtable_lookup_info_t *lookup_funcs = NULL; > - int32_t count = dpcls_subtable_lookup_info_get(&lookup_funcs); > - if (count < 0) { > - unixctl_command_reply_error(conn, "error getting lookup names"); > - return; > - } > - > - /* Add all lookup functions to reply string. */ > struct ds reply = DS_EMPTY_INITIALIZER; > - ds_put_cstr(&reply, "Available lookup functions (priority : name)\n"); > - for (int i = 0; i < count; i++) { > - ds_put_format(&reply, " %d : %s\n", lookup_funcs[i].prio, > - lookup_funcs[i].name); > - } > + > + dp_dpcls_impl_print_stats(&reply); > unixctl_command_reply(conn, ds_cstr(&reply)); > ds_destroy(&reply); > } > @@ -8940,6 +8928,10 @@ dpcls_destroy_subtable(struct dpcls *cls, struct > dpcls_subtable *subtable) > pvector_remove(&cls->subtables, subtable); > cmap_remove(&cls->subtables_map, &subtable->cmap_node, > subtable->mask.hash); > + > + dpcls_subtable_get_best_impl(subtable->mf_bits_set_unit0, > + subtable->mf_bits_set_unit1, > + subtable->lookup_func, false); > ovsrcu_postpone(dpcls_subtable_destroy_cb, subtable); > } > > @@ -8987,8 +8979,8 @@ dpcls_create_subtable(struct dpcls *cls, const struct > netdev_flow_key *mask) > * The function is guaranteed to always return a valid implementation, > and > * possibly an ISA optimized, and/or specialized implementation. > */ > - subtable->lookup_func = dpcls_subtable_get_best_impl(unit0, unit1); > - > + subtable->lookup_func = dpcls_subtable_get_best_impl(unit0, unit1, NULL, > + true); > 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); > @@ -9028,8 +9020,11 @@ dpcls_subtable_lookup_reprobe(struct dpcls *cls) > uint32_t u0_bits = subtable->mf_bits_set_unit0; > uint32_t u1_bits = subtable->mf_bits_set_unit1; > void *old_func = subtable->lookup_func; > - subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits, > u1_bits); > - subtables_changed += (old_func != subtable->lookup_func); > + subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits, > u1_bits, > + old_func, true); > + if (old_func != subtable->lookup_func) { > + subtables_changed += 1; > + } > } > pvector_publish(pvec); > > diff --git a/tests/pmd.at b/tests/pmd.at > index c875a744f..e61bb27d3 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -1091,11 +1091,11 @@ AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface > p1 type=dummy-pmd]) > > AT_CHECK([ovs-vsctl show], [], [stdout]) > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], > [], [dnl > - 1 : generic > + generic (Use count: 0, Priority: 1) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > autovalidator], [], [dnl > - 0 : autovalidator > + autovalidator (Use count: 0, Priority: 0) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 3], > [0], [dnl > @@ -1103,7 +1103,7 @@ Lookup priority change affected 0 dpcls ports and 0 > subtables. > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > autovalidator], [], [dnl > - 3 : autovalidator > + autovalidator (Use count: 0, Priority: 3) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 4], [0], > [dnl > @@ -1111,7 +1111,7 @@ Lookup priority change affected 0 dpcls ports and 0 > subtables. > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], > [], [dnl > - 4 : generic > + generic (Use count: 0, Priority: 4) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 8], [0], > [dnl > @@ -1119,7 +1119,7 @@ Lookup priority change affected 0 dpcls ports and 0 > subtables. > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], > [], [dnl > - 8 : generic > + generic (Use count: 0, Priority: 8) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 8], > [0], [dnl > @@ -1127,7 +1127,7 @@ Lookup priority change affected 0 dpcls ports and 0 > subtables. > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep > autovalidator], [], [dnl > - 8 : autovalidator > + autovalidator (Use count: 0, Priority: 8) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 0], [0], > [dnl > @@ -1135,7 +1135,7 @@ Lookup priority change affected 0 dpcls ports and 0 > subtables. > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], > [], [dnl > - 0 : generic > + generic (Use count: 0, Priority: 0) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 255], [0], > [dnl > @@ -1143,7 +1143,7 @@ Lookup priority change affected 0 dpcls ports and 0 > subtables. > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], > [], [dnl > - 255 : generic > + generic (Use count: 0, Priority: 255) > ]) > > AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic -1], [2], > -- > 2.25.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
