[ovs-dev] [v4 03/12] dpif-netdev: Add study function to select the best mfex function

Eelco Chaudron echaudro at redhat.com
Wed Jun 30 10:07:29 UTC 2021

On 30 Jun 2021, at 11:43, Van Haaren, Harry wrote:

>> -----Original Message-----
>> From: Flavio Leitner <fbl at sysclose.org>
>> Sent: Tuesday, June 29, 2021 7:11 PM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>
>> Cc: Eelco Chaudron <echaudro at redhat.com>; Amber, Kumar
>> <kumar.amber at intel.com>; dev at openvswitch.org; i.maximets at ovn.org
>> Subject: Re: [ovs-dev] [v4 03/12] dpif-netdev: Add study function to select the best
>> mfex function
>> On Tue, Jun 29, 2021 at 04:32:05PM +0000, Van Haaren, Harry wrote:
>>>> -----Original Message-----
>>>> From: dev <ovs-dev-bounces at openvswitch.org> On Behalf Of Eelco Chaudron
>>>> Sent: Tuesday, June 29, 2021 1:38 PM
>>>> To: Amber, Kumar <kumar.amber at intel.com>
>>>> Cc: dev at openvswitch.org; i.maximets at 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?
> That's a good question. Today the command doesn't give us per-PMD thread
> control. Study can indeed result in different PMDs having different MFEX funcs.
>> 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.
> Sure they do; the "pmd-cX" and "pmd-cY" below show what datapath thread selects what function.
> Note that the first line is from the OVS command thread, which notes that "study" was selected.
> The following two prints are from each datapath thread, noting the resulting function chosen by study.
> 2021-06-30T09:05:41Z|00134|dpif_netdev|INFO|Miniflow implementation set to study.
> 2021-06-30T09:05:41Z|00001|dpif_mfex_extract_study(pmd-cX/id:X)|INFO|MFEX study chose impl avx512_vbmi_ipv4_udp: (hits 128/128 pkts)
> 2021-06-30T09:05:41Z|00001|dpif_mfex_extract_study(pmd-cY/id:Y)|INFO|MFEX study chose impl avx512_vbmi_ipv4_udp: (hits 128/128 pkts)

And with the updated miniflow-parser-get we should be able to see it after the logs have wrapped.

>>> 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'?
> I was expecting OVS users to be aware of what L2-4 traffic they're
> running, and to per-instance configure that statically for all datapath
> threads, for example by running the command below:
> $ ovs-appctl  dpif-netdev/miniflow-parser-set avx512_ipv4_udp
> There is an assumption here that all datapath threads handle
> the same outer traffic type. If that's not the case, we cannot manually
> set different MFEX impls to different pmd threads today, as your lab
> to production requirement requests above.
> If we add an optional PMD thread id parameter, we can support this:
> $ ovs-appctl  dpif-netdev/miniflow-parser-set avx512_ipv4_udp <packet_count_to_study> <pmd thread idx>

I think if we allow study to set it per PMD thread, we should support the pmd thread for manual configuration.
We also might need to re-think the command to make sure packet_count_to_study is only needed for the study command.
So the help text might become something like:

dpif-netdev/miniflow-parser-set {miniflow_implementation_name | study [pkt_cnt]} [dp] [pmd_core]

More information about the dev mailing list