> -----Original Message-----
> From: Eelco Chaudron <echau...@redhat.com>
> Sent: Thursday, October 21, 2021 1:04 PM
> To: Amber, Kumar <kumar.am...@intel.com>
> Cc: ovs-dev@openvswitch.org; ktray...@redhat.com; i.maxim...@ovn.org;
> Stokes, Ian <ian.sto...@intel.com>; f...@sysclose.org; Van Haaren, Harry
> <harry.van.haa...@intel.com>
> Subject: Re: [PATCH v2] dpcls: Change info-get function to fetch dpcls usage
> stats.
>
> Hi Kumar,
>
> See inline comments below…
>
> //Eelco
Hey Eelco,
Thanks for reviewing; comments/discussion inline.
> On 20 Oct 2021, at 6:52, 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 <kumar.am...@intel.com>
> > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
> > Co-authored-by: Harry van Haaren <harry.van.haa...@intel.com>
<snip>
> > 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,
> > + uint32_t will_use_result)
>
> Why uint32_t and not a boolean?
bool is fine too, can change.
> > {
> > /* Iter over each subtable impl, and get highest priority one. */
> > int32_t prio = -1;
> > + uint32_t i;
> > const char *name = NULL;
> > + uint32_t best_idx = 0;
> > + uint32_t usage_cnt = 0;
> > 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++) {
> > int32_t probed_prio = subtable_lookups[i].prio;
> > + dpcls_subtable_lookup_func probed_func;
>
> Add extra blanc line here
Can do.
> > + probed_func = subtable_lookups[i].probe(u0_bit_count,
> > + u1_bit_count);
>
> Indentation is off
Will fix.
> > + 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) {
>
> Should this check also not include “&& will_use_result“?
Nope. When destroying a subtable, we want to decrement the counter, even when
the returned func ptr will not be used.
> Guess you overload this function to do a counter update, which I do not like
> (see comments below).
Yes - correct. I'd prefer keep separate APIs too, but in reality the
implementation to increment a counter
would require a search through all subtables (which we've already just done).
So iterating all dpcls implementations
a second time, doing a brute-force search in order to increment a variable
seemed too much effort, as well as
maintaining 2x implementations.
The goal of adding the "will use result" parameter (overloading API) was to
reduce code maintenance effort...
> > + atomic_read_relaxed(&subtable_lookups[i].usage_cnt,
> > &usage_cnt);
> > + usage_cnt--;
> > + atomic_store_relaxed(&subtable_lookups[i].usage_cnt,
> > usage_cnt);
>
> Not sure what you are trying here, but looks like you make an atomic
> operation,
> not atomic ;)
>
> We have the following APIs which I think you should use;
> atomic_count_init()/atomic_count_inc()/atomic_count_dec().
@Amber; would you investigate here and update to use atomic_count_inc/dec()?
> > - VLOG_DBG("Subtable lookup function '%s' with units (%d,%d), priority
> %d\n",
> > - name, u0_bit_count, u1_bit_count, prio);
> > + /* Update stats for usage. */
> > + if (will_use_result) {
> > + atomic_read_relaxed(&subtable_lookups[best_idx].usage_cnt,
> &usage_cnt);
> > + usage_cnt++;
> > + atomic_store_relaxed(&subtable_lookups[best_idx].usage_cnt,
> usage_cnt);
> > + } else {
> > + VLOG_DBG(
> > + "Subtable lookup function '%s' with units (%d,%d), priority
> > %d\n",
> > + name, u0_bit_count, u1_bit_count, prio);
> > + }
>
> Why not always show the log? Guess now you only show it when not using the
> return value?
Will investigate always showing log.
> > /* Programming error - we must always return a valid func ptr. */
> > ovs_assert(best_func != NULL);
> >
> > 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");
>
> Would be nice to do some qsort() to show the entries based on priority/name?
I don’t think its worth the complexity. The order is always the order as they
are listed in the code.
What value does sorting have? It makes complexity of Unit tests higher, as we'd
have to re-order the implementations there too.
> > + for (int i = 0; i < count; i++) {
> > + ds_put_format(reply, " %s (Use count: %d, Priority: %d",
> > + lookup_funcs[i].name, 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..13431a3fb 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_uint32_t usage_cnt;
>
> Guess this needs to be atomic_count
Sure will rename.
> > 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.
> > + */
>
> See below, as I do not like the API.
See discussion/reply on API/design and implementation above.
> > 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,
> > + uint32_t will_use);
> >
> > /* 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..c4ca3ce51 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -911,21 +911,8 @@ 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);
> > - }
>
> Add extra enter between variable declaration and code.
Will fix.
> > + dp_dpcls_impl_print_stats(&reply);
> > unixctl_command_reply(conn, ds_cstr(&reply));
> > ds_destroy(&reply);
> > }
> > @@ -8940,6 +8927,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, 0);
>
> First, the 0, should probably be a false bool. But in general, I do not like
> this API
> overload. Why would you call a dpcls_subtable_get_best_impl() to reset a
> counter? Can we not add a new API in form or
> dpcls_update_impl_use_count(struct dpcls_subtable *ubtable, bool increase), or
> something else thats is more clear?
As per above, the "dpcls_update_impl_use_count()" would require a brute-force
search through the tables again.
By incorporating the "increment" in the API, we can just iterate the
implementations once.
I think it is worth the complexity to avoid re-iterating implementations a 2nd
time.
> > ovsrcu_postpone(dpcls_subtable_destroy_cb, subtable);
> > }
> >
> > @@ -8987,8 +8978,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,
> > + 1);
>
> If dpcls_subtable_get_best_impl() would always increase the use count, no
> changes should be needed here.
I see where you're going with this approach - see above comments.
<snip remaining patch & suggestions of the 2 apis approach>
Would documenting the reasoning clearly as to why the API is overloaded, and
keeping
implementation (1 function, overloeaded with will_use_result) the same as is
today be acceptable?
Regards, -Harry
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
