On 30 Jun 2021, at 11:43, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Flavio Leitner <f...@sysclose.org>
>> Sent: Tuesday, June 29, 2021 7:11 PM
>> To: Van Haaren, Harry <harry.van.haa...@intel.com>
>> Cc: Eelco Chaudron <echau...@redhat.com>; Amber, Kumar
>> <kumar.am...@intel.com>; 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
>>
>> 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?
>
> 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]
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
