On Tue, Jun 29, 2021 at 04:32:05PM +0000, Van Haaren, Harry wrote:
> > -----Original Message-----
> > From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of Eelco Chaudron
> > Sent: Tuesday, June 29, 2021 1:38 PM
> > To: Amber, Kumar <kumar.am...@intel.com>
> > Cc: d...@openvswitch.org; i.maxim...@ovn.org
> > Subject: Re: [ovs-dev] [v4 03/12] dpif-netdev: Add study function to select
> > the best
> > mfex function
> >
> > More comments below. FYI I’m only reviewing right now, no testing.
>
> Sure, thanks for reviews.
>
> > On 17 Jun 2021, at 18:27, Kumar Amber wrote:
>
> <snip patch commit and some code>
>
> > > +/* Allocate per thread PMD pointer space for study_stats. */
> > > +static inline struct study_stats *
> > > +get_study_stats(void)
> > > +{
> > > + struct study_stats *stats = study_stats_get();
> > > + if (OVS_UNLIKELY(!stats)) {
> > > + stats = xzalloc(sizeof *stats);
> > > + study_stats_set_unsafe(stats);
> > > + }
> > > + return stats;
> > > +}
> > > +
> >
> > Just got a mind-meld with the code, and realized that the function might be
> > different
> > per PMD thread due to this auto mode (and autovalidator mode in the previous
> > patch).
> >
> > This makes it only stronger that we need a way to see the currently
> > selected mode,
> > and not per datapath, but per PMD per datapath!
>
> Study depends on the traffic pattern, so yes you're correct that it depends.
> The study command was added after community suggested user-experience
> would improve if the user doesn't have to provide an exact miniflow profile
> name.
>
> Study studies the traffic running on that PMD, compares all MFEX impls, and
> prints out
> hits. It selects the _first_ implementation that surpasses the threshold of
> packets.
>
> Users are free to use the more specific names of MFEX impls instead of "study"
> for fine-grained control over the MFEX impl in use, e.g.
>
> ovs-appctl dpif-netdev/miniflow-parser-set avx512_vbmi_ipv4_udp
>
> > Do we also need a way to set this per PMD?
>
> I don't feel there is real value here, but we could investigate adding an
> optional parameter to the command indicating a PMD thread IDX to set?
> We have access to "pmd->core_id" in our set() function, so limiting changes
> to a specific PMD thread can be done ~ easily... but is it really required?
I think the concern here (at least from my side) is that users can
set the algorithm globally or per DP, not per PMD. However, the
study can set different algorithms per PMD. For example, say that
'study' indicates that alg#1 for PMD#1 and alg#2 for PMD#2 in the
lab. Now we want to move to production and make that selection
static, how can we do that?
If we set study, how do we tell from the cmdline the algorithm
chose for each PMD? Another example of the same situation: if
we always start with 'study' and suddenly there is a traffic
processing difference. How one can check what is different in
the settings? The logs don't tell which PMD was affected.
> Perfect is the enemy of good... I'd prefer focus on getting existing code
> changes merged,
> and add additional (optional) parameters in future if deemed useful in real
> world testing?
True. Perhaps we have different use cases in mind. How do you expect
users to use this feature? Do you think production users will always
start with 'study'?
Thanks,
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
