[ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle count and rebased patches

Ilya Maximets i.maximets at samsung.com
Tue Jan 9 07:19:43 UTC 2018 

Hello everyone. I just returned from the holidays.
Thanks for working on this. Find my replies inline.

Best regards, Ilya Maximets.

On 08.01.2018 20:58, Kevin Traynor wrote:
> On 01/08/2018 05:31 PM, Stokes, Ian wrote:
>>>
>>> Hi Ian,
>>>
>>>> -----Original Message-----
>>>> From: Stokes, Ian [mailto:ian.stokes at intel.com]
>>>> Sent: Monday, 08 January, 2018 17:09
>>>> To: Jan Scheurich <jan.scheurich at ericsson.com>; Kevin Traynor
>>>> <ktraynor at redhat.com>; Ilya Maximets <i.maximets at samsung.com>
>>>> Cc: dev at openvswitch.org
>>>> Subject: RE: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle
>>>> count and rebased patches
>>>>
>>>>> Hi Ian,
>>>>>
>>>>> That's why I brought up my original question before Christmas, but
>>>>> apparently too late ☹.
>>>>
>>>> Apologies, Christmas was a bit hectic with the last push for output
>>> batching and finishing for the holidays so I missed following up on it.
>>>
>>> No worries, I understand. Actually Ilya responded and agreed to my
>>> proposal for deciding on an order. But we didn't have time to discuss the
>>> best order. He had v9 out before Christmas based on dpdk_merge (later
>>> merged to master) and I took that for rebase.

I have a few concerns about this rebase. It wan't trivial and you made few
decisions which I would argue with. For example, adding a new parameter
to 'dp_netdev_process_rxq_port'. It's not needed. I'll reply later to
the patch itself. There are also few style issues. I personally prefer to
rebase my patches myself to avoid such situations where I disagree with
rebase made by someone else especially without mentioning in commit-message.

>>>
>>>>>
>>>>> Myself, was hoping that we could get all three changes: PMD
>>>>> performance metrics, time-based tx batching and Kevin's enhancement
>>>>> for the pmd-rxq- show command into 2.9.
>>>>>
>>>>
>>>> I think this is the ideal situation, but does require sign off from
>>>> Ilya and Kevin as it will be their features it will impact on, I guess
>>> they can answer and give an idea on bandwidth to cooperate on something
>>> like this.
>>>>
>>>>> PMD performance metrics are for sure around long enough to warrant
>>> that.

It's not a first time I hear that something is ready to be merged because
it was submitted long time ago and has few iterations of rebasing. But
the truth is that our "OVS-DPDK" community is really small and we don't have
enough active reviewers/time to handle all the huge patch series that
constantly appears on the mail list. It's sad, but true.

About "PMD performance metrics" patch-set: v4 is the only iteration that was
really reviewed. And these reviews was mostly on a high level. At a first
glance v5 has at least 2 real bugs and, IMHO, design of some points is not
really good. I'll reply with details to the series soon.

>>>>> And they are highly wanted too.
>>>>
>>>> To my mind the detailed PMD logs patchset has a lot of content to work
>>>> through and there are reviews in progress as well as re-work requests
>>>> from previous reviews of the v4. I was waiting to see these completed
>>> before focusing on it myself as I haven't had the bandwidth to date to
>>> take it on.
>>>
>>> All pending review comments have been incorporated in v5 posted on Jan 4th
>>> (http://patchwork.ozlabs.org/cover/855572/). That version is rebased to
>>> master after merging  the non-tima-based output batching.
>>
>> Apologies, Jan, I've spotted the v5 now (it was v4 assigned to myself in patchwork so that was what I had been planning to look at).
>>
>>>
>>> I am just now preparing a v6 that adds the missing documentation for the
>>> new commands to the ovs-vswitchd man page.
>>
>> Excellent, is there an ETA on the v6? I think Billy is already reviewing the v5 but I'll confirm, the switch to v6 should be painless if it's mainly documentation.
>>
>>>
>>>>
>>>>>
>>>>> Since all three are heavily affecting same parts of the code the
>>>>> order of merging matters a lot. If we want to make this happen in
>>>>> reasonable time we need to avoid constant manual re-basing for all of
>>> us.
>>>>>
>>>>> That’s why I have taken the initiative to serialize them into one
>>>>> particular order for merging. That was a painful exercise and I am
>>>>> not looking forward to doing it again in a different order.
>>>>
>>>> Agreed and thanks for taking the initiative. I do think this is
>>> important if all three are to make it in.
> 
> +1. thanks for this Jan.
> 
>>>>
>>>>>
>>>>> So I would greatly appreciate if we could agree on the proposed
>>>>> order and discuss how we review/test the resulting overall
>>>>> contribution with the ambition to get all parts into 2.9.
>>>>
>>>> I'm open to others input here, based on the points you've raised I
>>>> would suggest the following (only a suggestion, please feel free to
>>>> counter):
>>>>
>>>> 1: Output Time batching (it's v9 and is well understood at this point,
>>>> I would think would be little re-work needed if the cases suit the PMD
>>> balancing already in place).
>>>> 2: Detailed PMD logs (from what I understand there is a review in
>>>> press from Billy, and re-work requests from Aaron, we could roll these
>>> changes into the next rebase on top of the time output batching).
>>>
>>> As stated above, v5 is already out since Thursday and used as my #1.
>>
>> Ok.
>>
>>>
>>>> 3: Kevin's PMD patches: probably the smallest of the set and lowest risk
>>> IMO as you seem familiar already with these Jan?
>>>
>>> The patch was IMHU unnecessarily big and is now, after my simplifications,
>>> rather small.
>>
>> Ok, that lowers the risk a little bit more, sounds good to me but will wait for an ACK from Kevin on this before signing off myself after reviewing/testing.
>>
> 
> I like the new scheme of not collecting the idle cycles per rxq,
> especially with the changes that will be there now for tx-batching/pmd
> metrics.  There was some data structure simplification in the original
> patches which got dropped, so I'd like to take a look at that area and
> see if there's something that I can add. Possibly a couple of minor
> issues/changes too, but I need to review more/test.
> 
>>>
>>>>
>>>> Does this seem acceptable?
>>>
>>> Actually, as I have already prepared the patches in the other order
>>>   1. Detailed PMD metrics (Jan)
>>>   2. Time-based output batching (Ilya)
>>>   3. Refactor cycle counting (Jan)
>>>   4. PMD load in pmd-rxq-show (Kevin/Jan) I would prefer not to swap 1 and
>>> 2. I am afraid it will imply a few extra hours to sort out the conflicts
>>> again. Time none of us has and which would be better spent on
>>> reviewing/testing.
>>
>> Ok let's avoid the overhead re-work and go with the approach you've suggested above.
>>
> 
> Yes, but it may actually be possible to take 4 out of the dependency
> chain, I'll check that - leave it there for now.
> 
>>>
>>> The end result should anyway be the same. This is what we need to focus
>>> on. We are just talking about different intermediate steps on the way that
>>> will not live for more than a few hours (or days).
>>
>> Sure.
>>
>>>
>>>> It's probably a good topic for the community meeting Wednesday but I'd
>>>> like to try and get agreement before then if we are to coordinate to get
>>> these upstreamed.
>>>
>>> I hope we can agree on a common approach earlier than the meeting. If mail
>>> I too slow, I can call for a short dedicated Skype call to discuss and
>>> agree.
>>
>> Ok, I think you're approach looks good to me, I'll start reviewing the time based output batching based on your RFC tomorrow, I'll coordinate with Billy as regards the review and testing of the v5 detailed logs with the understanding these are pre requisite. I see Kevin is already testing and reviewing the PMD changes.
>>
>> Unless there are any objections (I don’t think there are) then let's go with this.

Jan, thanks for working on this. I really appreciate you contribution.
Current situation is that "1. Detailed PMD metrics (Jan)" needs code changes
and has a controversial design (I'll send my findings soon). Rebased by Jan
"2. Time-based output batching (Ilya)" needs few code changes and another rebase.
And I guess, that that patches wasn't really tested by someone except authors.
I didn't review Kevin's series yet.
Combining all the above considerations and very limited number of participants
I'm afraid that we may not finish this work for 2.9 release. And we'll have no
new features at all.

>>
> 
> +1
> 
> thanks,
> Kevin.
> 
>> Thanks again for bringing this up. 
>>
>> Ian
>>>
>>>>
>>>> Ian
>>>>>
>>>>> Thanks, Jan
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ovs-dev-bounces at openvswitch.org
>>>>>> [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Stokes, Ian
>>>>>> Sent: Monday, 08 January, 2018 16:04
>>>>>> To: Jan Scheurich <jan.scheurich at ericsson.com>; Kevin Traynor
>>>>>> <ktraynor at redhat.com>; Ilya Maximets <i.maximets at samsung.com>
>>>>>> Cc: dev at openvswitch.org
>>>>>> Subject: Re: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle
>>>>>> count and rebased patches
>>>>>>
>>>>>>> Hi guys,
>>>>>>>
>>>>>>> It would be great to get your feedback on the proposal for
>>>>>>> combining (and
>>>>>>> simplifying) our patches.
>>>>>>>
>>>>>>> Do you agree with a) the cycle counting refactoring and b) the
>>>>>>> simplification of rxq pmd load calculation?
>>>>>>>
>>>>>>> For actual merge I would suggest to re-order the changes and
>>>>>>> take the cycle counting refactoring before the time-based output
>>>>>>> batching. But that is perhaps just a matter of taste affecting
>>>>>>> some intermediate commits and not so important for the end-
>>> result...
>>>>>>>
>>>>>>> How should we proceed with including these to the dpdk_merge
>>> branch?
>>>>>>> - One patch series at a time in the order now laid out in my RFC
>>>>>>> series
>>>>>>> - One large series comprising all 4 contributions?
>>>>>>>
>>>>>>
>>>>>> Hi Jan,
>>>>>>
>>>>>> From my side I was hoping to review/test Ilya's v9 time based
>>>>>> output batching first with a view to upstreaming that feature for
>>>>>> the 2.9
>>>>> release.
>>>>>>
>>>>>> My worry was that by attaching it to other feature changes it may
>>>>>> not
>>>>> get up streamed.
>>>>>>
>>>>>> I've only had a cursory look at the Kevin's percentage pad patches.
>>>>>>
>>>>>> I was thinking of working to the following on dpdk_merge
>>>>>>
>>>>>> 1: Review/Upstream Ilya's time based output.
>>>>>> https://patchwork.ozlabs.org/project/openvswitch/list/?series=1986
>>>>>> 5
>>>>>> 2: Review/Upstream Kevin's
>>>>>> https://patchwork.ozlabs.org/user/todo/openvswitch/?series=18301
>>>>>>
>>>>>> Just a question, in your mail below I see 'Detailed PMD
>>>>>> performance metrics and supervision' is included in [1], Billy
>>>>>> O'Mahony is currently reviewing the latest revision of this from
>>>>>> what I'm aware of,
>>>>> this a pre-requisite for you before upstreaming Ilya and Kevin's
>>> patches?
>>>>>>
>>>>>> I'd like to hear from Ilya on Kevin on this also? If they are
>>>>>> happy to combine the work and review/validate as a group then it may
>>> be possible.

It's hard to say what is better, I think that applying changes one by one
would be that best strategy we could work with. If we'll try to maintain
single huge patch-set we'll just overload one of us with constant rebasing
on every single comment. I'm afraid that code quality will suffer from that.

>>>>>>
>>>>>> Thanks
>>>>>> Ian
>>>>>>
>>>>>>> Regards, Jan
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: jan.scheurich at web.de [mailto:jan.scheurich at web.de] On
>>>>>>>> Behalf Of Jan Scheurich
>>>>>>>> Sent: Thursday, 04 January, 2018 21:03
>>>>>>>> To: dev at openvswitch.org
>>>>>>>> Cc: Jan Scheurich <jan.scheurich at ericsson.com>
>>>>>>>> Subject: [RFC PATCH 0/8] dpif-netdev: Refactor cycle count and
>>>>>>>> rebased patches
>>>>>>>>
>>>>>>>> This RFC patch series contains three contributions:
>>>>>>>>
>>>>>>>> 1. A rebase of Ilya's series "[PATCH v9,0/5] Output packet
>>>>>>>> batching
>>>>>>> (Time-based)."
>>>>>>>> (http://patchwork.ozlabs.org/cover/852003/) on top of "[PATCH
>>>>>>>> v5,0/3]
>>>>>>> dpif-netdev:
>>>>>>>> Detailed PMD performance metrics and supervision"
>>>>>>> (http://patchwork.ozlabs.org/cover/855572/).
>>>>>>>>
>>>>>>>> 2. Refactoring and simplification of the PMD cycle counting id
>>>>>>>> dpif-
>>>>>>> netdev.c.
>>>>>>>>
>>>>>>>> 3. A rebase and simplification of Kevin's patches
>>>>>>>> (http://patchwork.ozlabs.org/patch/847972/)
>>>>>>>> to display PMD usage per queue in the ovs-appctl pmd-rxq-show
>>>>> command.
>>>>>>>>
>>>>>>>> The patches pass check_patch and "make check" and I have done
>>>>>>>> some basic tests of the simplified rxq cycle counting and the
>>>>>>>> PMD usage
>>>>>>> reporting in pmd-rxq-show.
>>>>>>>>
>>>>>>>> This is my proposal how to combine the three existing patch
>>>>>>>> series together with the simplified cycle counting the into
>>>>>>>> branch dpdk_merge
>>>>>>> for release in OSV 2.9.
>>>>>>>>
>>>>>>>> Before merging the last two patches should probably be combined.
>>>>>>>>
>>>>>>>>
>>>>>>>> Ilya Maximets (5):
>>>>>>>>   dpif-netdev: Use microsecond granularity.
>>>>>>>>   dpif-netdev: Count cycles on per-rxq basis.
>>>>>>>>   dpif-netdev: Time based output batching.
>>>>>>>>   docs: Describe output packet batching in DPDK guide.
>>>>>>>>   NEWS: Mark output packet batching support.
>>>>>>>>
>>>>>>>> Jan Scheurich (2):
>>>>>>>>   dpif-netdev: Refactor cycle counting
>>>>>>>>   dpif-netdev: Add percentage of pmd/core used by each rxq.
>>>>>>>>
>>>>>>>> Kevin Traynor (1):
>>>>>>>>   dpif-netdev: Reset the rxq current cycle counter on reload.
>>>>>>>>
>>>>>>>>  Documentation/howto/dpdk.rst         |  12 ++
>>>>>>>>  Documentation/intro/install/dpdk.rst |  58 ++++++
>>>>>>>>  NEWS                                 |   3 +
>>>>>>>>  lib/dpif-netdev.c                    | 350
>>> ++++++++++++++++++++++--
>>>>> ----
>>>>>>> -------
>>>>>>>>  tests/pmd.at                         |  51 +++--
>>>>>>>>  vswitchd/vswitch.xml                 |  16 ++
>>>>>>>>  6 files changed, 349 insertions(+), 141 deletions(-)
>>>>>>>>
>>>>>>>> --
>>>>>>>> 1.9.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> dev mailing list
>>>>>> dev at openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

More information about the dev mailing list