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 <echau...@redhat.com>
> >> Sent: Friday, July 9, 2021 4:42 PM
> >> To: Amber, Kumar <kumar.am...@intel.com>
> >> Cc: Flavio Leitner <f...@sysclose.org>; Ferriter, Cian
> >> <cian.ferri...@intel.com>;
> >> ovs-dev@openvswitch.org; i.maxim...@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 <echau...@redhat.com>
> >>>> Sent: Friday, July 9, 2021 4:36 PM
> >>>> To: Amber, Kumar <kumar.am...@intel.com>
> >>>> Cc: Flavio Leitner <f...@sysclose.org>; Ferriter, Cian
> >>>> <cian.ferri...@intel.com>; ovs-dev@openvswitch.org;
> >>>> i.maxim...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
