Re: [PATCH 1/6] service: reduce statistics overhead for parallel services

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2022-10-03 11:53, David Marchand wrote:
> On Mon, Oct 3, 2022 at 10:40 AM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>>
>> On 2022-10-03 10:06, David Marchand wrote:
>>> On Tue, Sep 6, 2022 at 6:17 PM Mattias Rönnblom
>>> <mattias.ronnblom@ericsson.com> wrote:
>>>>
>>>> Move the statistics from the service data structure to the per-lcore
>>>> struct. This eliminates contention for the counter cache lines, which
>>>> decreases the producer-side statistics overhead for services deployed
>>>> across many lcores.
>>>>
>>>> Prior to this patch, enabling statistics for a service with a
>>>> per-service function call latency of 1000 clock cycles deployed across
>>>> 16 cores on a Intel Xeon 6230N @ 2,3 GHz would incur a cost of ~10000
>>>> core clock cycles per service call. After this patch, the statistics
>>>> overhead is reduce to 22 clock cycles per call.
>>>>
>>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>>
>>> Re-reading the mail archive, I found that Morten acked the series
>>> (replying on patch 4).
>>> Mattias, such a series deserved a cover letter.
>>>
>>
>> Noted. Do you want a v2 with a cover letter?
>>
>>> How does this series fit with other patches from Harry?
>>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-9ac8e99afde9bc77&q=1&e=946b2bf5-b58c-4c02-b173-bba36b0b12f9&u=https%3A%2F%2Fpatchwork.dpdk.org%2Fproject%2Fdpdk%2Flist%2F%3Fseries%3D23959%26state%3D%2A
>>>
>>> Thanks.
>>>
>>
>> The patchset I submitted is made against the above-linked patchset.
>> Nothing in the patchset I submitted depends on those patches. I just
>> assumed that Harry's patches would have been merged at the point my
>> patchset was considered.
> 
> Harry v1 patch was indeed sent a while ago.
> But this v1 patch from Harry was superseded with a v2 and v3 series.
> There were comments on the v3 series from Harry.
> 
> 
> Now, looking at this series, without a cover letter, I had to guess.
> I saw it is linked to the v1 patch.
> I "assumed" it was then an alternative, since you had comments on
> Harry v3 patch, or at least Harry would reply and we could conclude.
> 

Sorry for failing to mention enough context in the patchset. I assumed 
it was Harry and not you that would resolve this issue. Harry's patchset 
fixes the statistics bug related to MT safe services, but does not 
address the performance issue discussed. So Harry's patchset makes sense 
on its own. It also adds a performance test case.

I believe the test case is the only thing left of Harry's improvements 
after my patchset is applied.

My patchset was meant as an improvement on what Harry already had done, 
not as an alternative.

This is all up the maintainer of course, but it seems to me that Harry's 
patchset should go in first, and then mine.

If Harry or you so prefer, I can rework my patchset to apply cleanly 
against current main (i.e., w/o Harry's patches).

> 
> So what do we do?
> Should I understand that your comments on Harry series can be ignored
> and I proceed with all this?
> 

My comments were minor, except those that relates to the issue that my 
follow-up patchset addresses.

> I hope it applies cleanly.
> 
>