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

[Flavio Leitner]

On Fri, Jul 09, 2021 at 01:55:25PM +0200, Eelco Chaudron wrote:
> 
> 
> 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.

I will jump straight to v7 and consider what has been asked in v6.
I think we can review v7 in parallel.

Thanks,
-- 
fbl