> 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:[email protected]] > >>>> Sent: Monday, 08 January, 2018 17:09 > >>>> To: Jan Scheurich <[email protected]>; Kevin Traynor > >>>> <[email protected]>; Ilya Maximets <[email protected]> > >>>> Cc: [email protected] > >>>> 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.
+1, unfortunately there's limited bandwidth and it's a trend that larger features/patchsets tend to be more 'controversial' for lack of a better word, they require more input and tend to be pushed out. Even in the past where it seems a patchset is ready to go a reviewer who only recently has time to review can object and push the work out further. It's frustrating but something I think as a community we need address. > > 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. If it helps here Ilya, there are ongoing reviews from Intel now on this as well (on the v5). However I see your also reviewing as well now on this. > > >>>>> 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. This goes back to my original fear of 1 large patchset and trying to get agreement on all aspects of it in time for 2.9. As such that is why I had suggested a different merge steps where time batching was first in the merge queue. In my opinion I'd like to focus on 1 feature and ensure it is upstreamed but as Jan has pointed out this puts a lot of re-work for rebasing on his side which would be good to avoid. Can I suggest we have a skype call to discuss further today if people are open to it? Ian > > >> > > > > +1 > > > > thanks, > > Kevin. > > > >> Thanks again for bringing this up. > >> > >> Ian > >>> > >>>> > >>>> Ian > >>>>> > >>>>> Thanks, Jan > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: [email protected] > >>>>>> [mailto:[email protected]] On Behalf Of Stokes, Ian > >>>>>> Sent: Monday, 08 January, 2018 16:04 > >>>>>> To: Jan Scheurich <[email protected]>; Kevin Traynor > >>>>>> <[email protected]>; Ilya Maximets <[email protected]> > >>>>>> Cc: [email protected] > >>>>>> 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=198 > >>>>>> 6 > >>>>>> 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: [email protected] [mailto:[email protected]] On > >>>>>>>> Behalf Of Jan Scheurich > >>>>>>>> Sent: Thursday, 04 January, 2018 21:03 > >>>>>>>> To: [email protected] > >>>>>>>> Cc: Jan Scheurich <[email protected]> > >>>>>>>> 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 > >>>>>> [email protected] > >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
