[ovs-dev] [v6 01/11] dpif-netdev: Add command line and function pointer for miniflow extract

[Eelco Chaudron]

On 9 Jul 2021, at 13:16, Amber, Kumar wrote:

> Hi Eelco,
>
>> -----Original Message-----
>> From: Eelco Chaudron <echaudro at redhat.com>
>> Sent: Friday, July 9, 2021 4:42 PM
>> To: Amber, Kumar <kumar.amber at intel.com>
>> Cc: Flavio Leitner <fbl at sysclose.org>; Ferriter, Cian <cian.ferriter at intel.com>;
>> ovs-dev at openvswitch.org; i.maximets at ovn.org
>> Subject: Re: [ovs-dev] [v6 01/11] dpif-netdev: Add command line and function
>> pointer for miniflow extract
>>
>>
>>
>> On 9 Jul 2021, at 13:10, Amber, Kumar wrote:
>>
>>> Hi Eelco,
>>>
>>>> -----Original Message-----
>>>> From: Eelco Chaudron <echaudro at redhat.com>
>>>> Sent: Friday, July 9, 2021 4:36 PM
>>>> To: Amber, Kumar <kumar.amber at intel.com>
>>>> Cc: Flavio Leitner <fbl at sysclose.org>; Ferriter, Cian
>>>> <cian.ferriter at intel.com>; ovs-dev at openvswitch.org;
>>>> i.maximets at ovn.org
>>>> Subject: Re: [ovs-dev] [v6 01/11] dpif-netdev: Add command line and
>>>> function pointer for miniflow extract
>>>>
>>>>
>>>>
>>>> On 8 Jul 2021, at 16:01, Amber, Kumar wrote:
>>>>
>>>>> Hi Flavio,
>>>>>
>>>>> Thanks for the Review
>>>>> Replies are inline.
>>>>>
>>>>> <Snip>
>>>>>
>>>>>>> +miniflow_extract_func
>>>>>>> +dp_mfex_impl_get_default(void)
>>>>>>> +{
>>>>>>> +    /* For the first call, this will be NULL. Compute the compile time
>> default.
>>>>>>> +     */
>>>>>>> +    if (!default_mfex_func) {
>>>>>>> +
>>>>>>> +        VLOG_INFO("Default MFEX implementation is %s.\n",
>>>>>>> +                  mfex_impls[MFEX_IMPL_SCALAR].name);
>>>>>>> +        default_mfex_func =
>> mfex_impls[MFEX_IMPL_SCALAR].extract_func;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return default_mfex_func;
>>>>>>
>>>>>> Eelco asked to use VLOG_INFO_ONCE to avoid flooding the log, which
>>>>>> in the end will use a static variable. Perhaps it would be better
>>>>>> to define a static boolean like:
>>>>>>
>>>>>> miniflow_extract_func
>>>>>> dp_mfex_impl_get_default(void)
>>>>>> {
>>>>>>    /* For the first call, this will be NULL. Compute the compile time default.
>>>>>>     */
>>>>>>    static bool default_mfex_func_set = false;
>>>>>>
>>>>>>    if (OVS_UNLIKELY(!default_mfex_func_set)) {
>>>>>>        VLOG_INFO("Default MFEX implementation is %s.\n",
>>>>>>                  mfex_impls[MFEX_IMPL_SCALAR].name);
>>>>>>        // FIXME: Atomic set?
>>>>>>        default_mfex_func = mfex_impls[MFEX_IMPL_SCALAR].extract_func;
>>>>>>        default_mfex_func_set = true;
>>>>>>    }
>>>>>>
>>>>>>    return default_mfex_func;
>>>>>> }
>>>>>>
>>>>>
>>>>> Sound good taking into v7.
>>>>
>>>> As you already sent out a v7, I guess you mean v8?
>>>>
>>>> Are you planning to send out a v8 after you incorporate all Flavio's
>>>> comments? If so, I hold off on v7 and wait for v8.
>>>>
>>>
>>> The changes are in v7 itself.
>>
>> Ok, Anyway, I’ll wait for Flavio to finish his review before I start on v7 to avoid
>> having to look at v8 again.
>>
>
> You can review other patches in the series in the meanwhile 😊

But then I have again to review the v8, I have some other stuff to work on in the meantime ;)

I’m assuming Flavio will look at the whole set. Flavio can you ping us when you’re done.