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

[Amber, Kumar]

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 😊
> >> <SNIP>