> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Friday, October 30, 2020 10:48 AM
> To: McDaniel, Timothy <timothy.mcdan...@intel.com>
> Cc: 'dev@dpdk.org' <dev@dpdk.org>; Carrillo, Erik G
> <erik.g.carri...@intel.com>; Eads, Gage <gage.e...@intel.com>; Van Haaren,
> Harry <harry.van.haa...@intel.com>; 'jer...@marvell.com'
> <jer...@marvell.com>; 'david.march...@redhat.com'
> <david.march...@redhat.com>
> Subject: Re: [PATCH v5 00/23] Add DLB2 PMD
>
> 30/10/2020 16:35, McDaniel, Timothy:
> > From: Thomas Monjalon <tho...@monjalon.net>
> > > 30/10/2020 12:58, McDaniel, Timothy:
> > > > From: McDaniel, Timothy
> > > > > From: Thomas Monjalon <tho...@monjalon.net>
> > > > > > 30/10/2020 10:43, Timothy McDaniel:
> > > > > > > - note that the code still uses its private byte-encoded versions
> > > > > > > of
> > > > > > > umonitor/umwait, rather than the new functions in the power
> > > > > > > patch that are built on top of those intrinsics. This is
> > > > > > > intentional.
> > > > > >
> > > > > > Why? Now these intrinsics are available in the main branch.
> > > > > > We should avoid duplicating such code.
> > > > > >
> > > > > >
> > > > >
> > > > > I had asked that the low level intrinsics (UMWAIT/UMONITOR) be split
> out so
> > > > > that DLB/DLB2 could use them instead of its own private byte-encoded
> > > versions,
> > > > > but instead we have these wrappers that call the low level intrinsics.
> Those
> > > > > wrappers
> > > > > introduce additional overhead that is not required for DLB/DLB2. I
> > > > > have a
> > > > > meeting with Ma Liang on Monday to discuss.
> > > >
> > > > I thought the ask of DLB was to just substitute the low level
> umwait/umonitor
> > > byte
> > > > encoded instructions DLB has defined privately with similar byte-encoded
> > > instructions defined in the power
> > > > patch. The power patch does not directly expose those, which is why I
> > > > did
> not
> > > update DLB/DLB2.
> > > > The power patch does have the advantage of centralizing the race
> avoidance
> > > > logic, which is a good thing for any PMD that wishes to take advantage
> > > > of
> > > umwait/umonitor.
> > >
> > > So you mean the overhead is a good thing?
> > >
> > > > Sorry for the confusion. I just misunderstood what was being asked of
> > > > DLB
> in
> > > regard to switching over.. That being said,
> > > > I am willing to convert DLB/DLB2 to use rte_power_monitor(...) in a
> > > > future
> > > patch-set.
> > >
> > > Why not now?
> > >
> > > Indeed there is a confusion and it looks like a lot of novlang
> > > to exit from the situation.
> > > We'll wait a clear decision with facts.
> > >
> >
> > Hi Thomas,
> >
> > I have updated DLB and DLB2 to use rte_power_monitor(...), and those
> patches are
> > ready if you are willing to accept them and the 3 power patches.
> >
> > For the sake of consistency, I see the benefit of using the power patch,
> > even if
> it is
> > slightly less efficient that the DLB-specific implementation that I
> > currently
> have.
> > We have already encountered an empty queue, so this is no longer fast path
> for the PMD.
>
> I am really concerned that the API in EAL is not the most efficient.
> Why is that? Can we improve the EAL API?
>
>From an efficiency perspective, I only noticed 2 things.
1) The size check and associated logic. This could be avoided if we had _8,
_16, _32, _64 variants instead of 1 function that handled all possibilities,
but even that may be a wash
with branch prediction
2) The spinlock - but this observation was my mistake, and was flawed. I was
looking at the rte_power_monitor_sync(...), and not rte_power_monitor(...), the
latter of which does not take a spinlock.
In summary, I am no longer concerned about efficiency and suitability for
DLB/DLB2.
Thanks,
Tim
