mbox series

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

Message ID 1515096166-16257-1-git-send-email-jan.scheurich@ericsson.com
Headers show
Series dpif-netdev: Refactor cycle count and rebased patches | expand

Message

Jan Scheurich Jan. 4, 2018, 8:02 p.m. UTC
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(-)

Comments

Jan Scheurich Jan. 8, 2018, 1:16 p.m. UTC | #1
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?

Regards, Jan

> -----Original Message-----
> From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On Behalf Of Jan Scheurich
> Sent: Thursday, 04 January, 2018 21:03
> To: dev@openvswitch.org
> Cc: Jan Scheurich <jan.scheurich@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
Stokes, Ian Jan. 8, 2018, 3:04 p.m. UTC | #2
> 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=19865
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.

Thanks
Ian

> Regards, Jan
> 
> > -----Original Message-----
> > From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On Behalf Of
> > Jan Scheurich
> > Sent: Thursday, 04 January, 2018 21:03
> > To: dev@openvswitch.org
> > Cc: Jan Scheurich <jan.scheurich@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
Jan Scheurich Jan. 8, 2018, 3:30 p.m. UTC | #3
Hi Ian,

That's why I brought up my original question before Christmas, but apparently too late ☹.

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.

PMD performance metrics are for sure around long enough to warrant that. And they are highly wanted too.

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.

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.

Thanks, Jan

> -----Original Message-----

> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian

> Sent: Monday, 08 January, 2018 16:04

> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> Cc: dev@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=19865

> 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.

> 

> Thanks

> Ian

> 

> > Regards, Jan

> >

> > > -----Original Message-----

> > > From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On Behalf Of

> > > Jan Scheurich

> > > Sent: Thursday, 04 January, 2018 21:03

> > > To: dev@openvswitch.org

> > > Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org

> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian Jan. 8, 2018, 4:09 p.m. UTC | #4
> 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.

> 

> 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.

> 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.

> 

> 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.

> 

> 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).
3: Kevin's PMD patches: probably the smallest of the set and lowest risk IMO as you seem familiar already with these Jan?

Does this seem acceptable?

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.

Ian
> 

> Thanks, Jan

> 

> > -----Original Message-----

> > From: ovs-dev-bounces@openvswitch.org

> > [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian

> > Sent: Monday, 08 January, 2018 16:04

> > To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> > <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> > Cc: dev@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=19865

> > 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.

> >

> > Thanks

> > Ian

> >

> > > Regards, Jan

> > >

> > > > -----Original Message-----

> > > > From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On Behalf

> > > > Of Jan Scheurich

> > > > Sent: Thursday, 04 January, 2018 21:03

> > > > To: dev@openvswitch.org

> > > > Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org

> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Jan Scheurich Jan. 8, 2018, 4:36 p.m. UTC | #5
Hi Ian,

> -----Original Message-----

> From: Stokes, Ian [mailto:ian.stokes@intel.com]

> Sent: Monday, 08 January, 2018 17:09

> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> Cc: dev@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.

> >

> > 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.

> > 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.

I am just now preparing a v6 that adds the missing documentation for the new commands to the ovs-vswitchd man page.

> 

> >

> > 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.

> 

> >

> > 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.

> 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.

> 

> 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.
 
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).

> 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.

> 

> Ian

> >

> > Thanks, Jan

> >

> > > -----Original Message-----

> > > From: ovs-dev-bounces@openvswitch.org

> > > [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian

> > > Sent: Monday, 08 January, 2018 16:04

> > > To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> > > <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> > > Cc: dev@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=19865

> > > 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.

> > >

> > > Thanks

> > > Ian

> > >

> > > > Regards, Jan

> > > >

> > > > > -----Original Message-----

> > > > > From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On Behalf

> > > > > Of Jan Scheurich

> > > > > Sent: Thursday, 04 January, 2018 21:03

> > > > > To: dev@openvswitch.org

> > > > > Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org

> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian Jan. 8, 2018, 5:31 p.m. UTC | #6
> 

> Hi Ian,

> 

> > -----Original Message-----

> > From: Stokes, Ian [mailto:ian.stokes@intel.com]

> > Sent: Monday, 08 January, 2018 17:09

> > To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> > <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> > Cc: dev@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.

> 

> > >

> > > 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.

> > > 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.

> >

> > >

> > > 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.

> 

> >

> > 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.

> 

> 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.

Thanks again for bringing this up. 

Ian
> 

> >

> > Ian

> > >

> > > Thanks, Jan

> > >

> > > > -----Original Message-----

> > > > From: ovs-dev-bounces@openvswitch.org

> > > > [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian

> > > > Sent: Monday, 08 January, 2018 16:04

> > > > To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> > > > <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> > > > Cc: dev@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.

> > > >

> > > > Thanks

> > > > Ian

> > > >

> > > > > Regards, Jan

> > > > >

> > > > > > -----Original Message-----

> > > > > > From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On

> > > > > > Behalf Of Jan Scheurich

> > > > > > Sent: Thursday, 04 January, 2018 21:03

> > > > > > To: dev@openvswitch.org

> > > > > > Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org

> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor Jan. 8, 2018, 5:58 p.m. UTC | #7
On 01/08/2018 05:31 PM, Stokes, Ian wrote:
>>
>> Hi Ian,
>>
>>> -----Original Message-----
>>> From: Stokes, Ian [mailto:ian.stokes@intel.com]
>>> Sent: Monday, 08 January, 2018 17:09
>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor
>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>
>>> Cc: dev@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.
>>
>>>>
>>>> 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.
>>>> 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.
> 

+1

thanks,
Kevin.

> Thanks again for bringing this up. 
> 
> Ian
>>
>>>
>>> Ian
>>>>
>>>> Thanks, Jan
>>>>
>>>>> -----Original Message-----
>>>>> From: ovs-dev-bounces@openvswitch.org
>>>>> [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian
>>>>> Sent: Monday, 08 January, 2018 16:04
>>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor
>>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>
>>>>> Cc: dev@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.
>>>>>
>>>>> Thanks
>>>>> Ian
>>>>>
>>>>>> Regards, Jan
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On
>>>>>>> Behalf Of Jan Scheurich
>>>>>>> Sent: Thursday, 04 January, 2018 21:03
>>>>>>> To: dev@openvswitch.org
>>>>>>> Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org
>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Jan. 9, 2018, 7:19 a.m. UTC | #8
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@intel.com]
>>>> Sent: Monday, 08 January, 2018 17:09
>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor
>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>
>>>> Cc: dev@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@openvswitch.org
>>>>>> [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian
>>>>>> Sent: Monday, 08 January, 2018 16:04
>>>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor
>>>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>
>>>>>> Cc: dev@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@web.de [mailto:jan.scheurich@web.de] On
>>>>>>>> Behalf Of Jan Scheurich
>>>>>>>> Sent: Thursday, 04 January, 2018 21:03
>>>>>>>> To: dev@openvswitch.org
>>>>>>>> Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian Jan. 9, 2018, 10:26 a.m. UTC | #9
> 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@intel.com]

> >>>> Sent: Monday, 08 January, 2018 17:09

> >>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> >>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> >>>> Cc: dev@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.


+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: ovs-dev-bounces@openvswitch.org

> >>>>>> [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian

> >>>>>> Sent: Monday, 08 January, 2018 16:04

> >>>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> >>>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> >>>>>> Cc: dev@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=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: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On

> >>>>>>>> Behalf Of Jan Scheurich

> >>>>>>>> Sent: Thursday, 04 January, 2018 21:03

> >>>>>>>> To: dev@openvswitch.org

> >>>>>>>> Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org

> >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Jan Scheurich Jan. 9, 2018, 12:42 p.m. UTC | #10
Hi guys,

Let's have a Skype call to align ourselves and the way forward.
I'm available 14-16 CET and 17-18 CET today. I'll send a tentative invite. Please reply.

I realize there are major comments on all three patch sets:

- Ilya has shared some architectural concerns on the PMD perf metrics v5 (v6 is just docs). I haven't had time to go through all of them yet.
- I don't believe the complex cycle counting in Ilya's v9 is maintainable. We need a much simpler approach (such as my suggested nestable cycle timers). And we should introduce that before the time-based tx batching.
- I have proposed to radically simplify Kevin's counting of total PMD cycles for the printout

None of them is ready for merging as it is today. If our goal is to get as many of these valuable features into 2.9 as possible, let's search for the most practical way of getting there.

My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one patch that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as possible.

Here’s my suggestions:
1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf
2. dpif-netdev: Refactor cycle counting (nestable cycle timer)
3a. Time-based tx batching
	dpif-netdev: Use microsecond granularity.
	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)
	dpif-netdev: Time based output batching.
	docs: Describe output packet batching in DPDK guide.
	NEWS: Mark output packet batching support.
3b. dpif-netdev: Add percentage of pmd/core used by each rxq.
3c. Detailed PMD Performance metrics
	dpif-netdev: Detailed performance stats for PMDs
	dpif-netdev: Detection and logging of suspicious PMD iterations

What do you think?

BR, Jan

> -----Original Message-----

> From: Stokes, Ian [mailto:ian.stokes@intel.com]

> Sent: Tuesday, 09 January, 2018 11:26

> To: Ilya Maximets <i.maximets@samsung.com>; Kevin Traynor <ktraynor@redhat.com>; Jan Scheurich <jan.scheurich@ericsson.com>

> Cc: dev@openvswitch.org; O Mahony, Billy <billy.o.mahony@intel.com>

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

> 

> > 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@intel.com]

> > >>>> Sent: Monday, 08 January, 2018 17:09

> > >>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> > >>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> > >>>> Cc: dev@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.

> 

> +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: ovs-dev-bounces@openvswitch.org

> > >>>>>> [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian

> > >>>>>> Sent: Monday, 08 January, 2018 16:04

> > >>>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor

> > >>>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>

> > >>>>>> Cc: dev@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=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: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On

> > >>>>>>>> Behalf Of Jan Scheurich

> > >>>>>>>> Sent: Thursday, 04 January, 2018 21:03

> > >>>>>>>> To: dev@openvswitch.org

> > >>>>>>>> Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org

> > >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Jan. 9, 2018, 1:11 p.m. UTC | #11
On 09.01.2018 15:42, Jan Scheurich wrote:
> Hi guys,
> 
> Let's have a Skype call to align ourselves and the way forward.
> I'm available 14-16 CET and 17-18 CET today. I'll send a tentative invite. Please reply.

Unfortunately, I'll not be able to attend today. It's already the end of my
working hours today and I still have some other stuff to do.

> 
> I realize there are major comments on all three patch sets:
> 
> - Ilya has shared some architectural concerns on the PMD perf metrics v5 (v6 is just docs). I haven't had time to go through all of them yet.
> - I don't believe the complex cycle counting in Ilya's v9 is maintainable. We need a much simpler approach (such as my suggested nestable cycle timers). And we should introduce that before the time-based tx batching.
> - I have proposed to radically simplify Kevin's counting of total PMD cycles for the printout
> 
> None of them is ready for merging as it is today. If our goal is to get as many of these valuable features into 2.9 as possible, let's search for the most practical way of getting there.
> 
> My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one patch that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as possible.
> 
> Here’s my suggestions:
> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf
> 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)
> 3a. Time-based tx batching
> 	dpif-netdev: Use microsecond granularity.
> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)
> 	dpif-netdev: Time based output batching.
> 	docs: Describe output packet batching in DPDK guide.
> 	NEWS: Mark output packet batching support.
> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.
> 3c. Detailed PMD Performance metrics
> 	dpif-netdev: Detailed performance stats for PMDs
> 	dpif-netdev: Detection and logging of suspicious PMD iterations
> 
> What do you think?

Basically, this looks good to me. But I still think that we should work on that
step-by-step not tying to make all the work at once. This will save time of
rebasing on intermediate versions of patches.
I'll try to spend some time from the rest of today to check out "nestable cycle timers".
Would like to see fixed patch from #1 and a proper patch for #2.
Step #3a should not be hard.

Thanks again.
Best regards, Ilya Maximets.

> 
> BR, Jan
> 
>> -----Original Message-----
>> From: Stokes, Ian [mailto:ian.stokes@intel.com]
>> Sent: Tuesday, 09 January, 2018 11:26
>> To: Ilya Maximets <i.maximets@samsung.com>; Kevin Traynor <ktraynor@redhat.com>; Jan Scheurich <jan.scheurich@ericsson.com>
>> Cc: dev@openvswitch.org; O Mahony, Billy <billy.o.mahony@intel.com>
>> Subject: RE: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle count and rebased patches
>>
>>> 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@intel.com]
>>>>>>> Sent: Monday, 08 January, 2018 17:09
>>>>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor
>>>>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>
>>>>>>> Cc: dev@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.
>>
>> +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: ovs-dev-bounces@openvswitch.org
>>>>>>>>> [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Stokes, Ian
>>>>>>>>> Sent: Monday, 08 January, 2018 16:04
>>>>>>>>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Kevin Traynor
>>>>>>>>> <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>
>>>>>>>>> Cc: dev@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=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: jan.scheurich@web.de [mailto:jan.scheurich@web.de] On
>>>>>>>>>>> Behalf Of Jan Scheurich
>>>>>>>>>>> Sent: Thursday, 04 January, 2018 21:03
>>>>>>>>>>> To: dev@openvswitch.org
>>>>>>>>>>> Cc: Jan Scheurich <jan.scheurich@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@openvswitch.org
>>>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Jan Scheurich Jan. 9, 2018, 1:30 p.m. UTC | #12
> -----Original Message-----

> From: Ilya Maximets [mailto:i.maximets@samsung.com]

> Sent: Tuesday, 09 January, 2018 14:11

> To: Jan Scheurich <jan.scheurich@ericsson.com>; Stokes, Ian <ian.stokes@intel.com>; Kevin Traynor <ktraynor@redhat.com>

> Cc: dev@openvswitch.org; O Mahony, Billy <billy.o.mahony@intel.com>

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

> 

> On 09.01.2018 15:42, Jan Scheurich wrote:

> > Hi guys,

> >

> > Let's have a Skype call to align ourselves and the way forward.

> > I'm available 14-16 CET and 17-18 CET today. I'll send a tentative invite. Please reply.

> 

> Unfortunately, I'll not be able to attend today. It's already the end of my

> working hours today and I still have some other stuff to do.

> 

> >

> > I realize there are major comments on all three patch sets:

> >

> > - Ilya has shared some architectural concerns on the PMD perf metrics v5 (v6 is just docs). I haven't had time to go through all of them

> yet.

> > - I don't believe the complex cycle counting in Ilya's v9 is maintainable. We need a much simpler approach (such as my suggested

> nestable cycle timers). And we should introduce that before the time-based tx batching.

> > - I have proposed to radically simplify Kevin's counting of total PMD cycles for the printout

> >

> > None of them is ready for merging as it is today. If our goal is to get as many of these valuable features into 2.9 as possible, let's search

> for the most practical way of getting there.

> >

> > My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one patch

> that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as possible.

> >

> > Here’s my suggestions:

> > 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf

> > 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)

> > 3a. Time-based tx batching

> > 	dpif-netdev: Use microsecond granularity.

> > 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)

> > 	dpif-netdev: Time based output batching.

> > 	docs: Describe output packet batching in DPDK guide.

> > 	NEWS: Mark output packet batching support.

> > 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.

> > 3c. Detailed PMD Performance metrics

> > 	dpif-netdev: Detailed performance stats for PMDs

> > 	dpif-netdev: Detection and logging of suspicious PMD iterations

> >

> > What do you think?

> 

> Basically, this looks good to me. But I still think that we should work on that

> step-by-step not tying to make all the work at once. This will save time of

> rebasing on intermediate versions of patches.


Fine with me as long as that doesn't stop review and testing of the not yet rebased
patches 3a-c. We need those tests and reviews to find and address any deficiencies
inherent in the feature (independent from rebasing).

> I'll try to spend some time from the rest of today to check out "nestable cycle timers".

> Would like to see fixed patch from #1 and a proper patch for #2.


I will try to send out patches for #1 (v6) and #2 this afternoon.

> Step #3a should not be hard.

> 


@Ian, Kevin and Billy: Should we anyway have a short Skype chat?

Regards, Jan
Kevin Traynor Jan. 9, 2018, 1:50 p.m. UTC | #13
On 01/09/2018 01:30 PM, Jan Scheurich wrote:
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Tuesday, 09 January, 2018 14:11
>> To: Jan Scheurich <jan.scheurich@ericsson.com>; Stokes, Ian <ian.stokes@intel.com>; Kevin Traynor <ktraynor@redhat.com>
>> Cc: dev@openvswitch.org; O Mahony, Billy <billy.o.mahony@intel.com>
>> Subject: Re: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle count and rebased patches
>>
>> On 09.01.2018 15:42, Jan Scheurich wrote:
>>> Hi guys,
>>>
>>> Let's have a Skype call to align ourselves and the way forward.
>>> I'm available 14-16 CET and 17-18 CET today. I'll send a tentative invite. Please reply.
>>
>> Unfortunately, I'll not be able to attend today. It's already the end of my
>> working hours today and I still have some other stuff to do.
>>
>>>
>>> I realize there are major comments on all three patch sets:
>>>
>>> - Ilya has shared some architectural concerns on the PMD perf metrics v5 (v6 is just docs). I haven't had time to go through all of them
>> yet.
>>> - I don't believe the complex cycle counting in Ilya's v9 is maintainable. We need a much simpler approach (such as my suggested
>> nestable cycle timers). And we should introduce that before the time-based tx batching.
>>> - I have proposed to radically simplify Kevin's counting of total PMD cycles for the printout
>>>
>>> None of them is ready for merging as it is today. If our goal is to get as many of these valuable features into 2.9 as possible, let's search
>> for the most practical way of getting there.
>>>
>>> My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one patch
>> that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as possible.
>>>
>>> Here’s my suggestions:
>>> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf
>>> 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)
>>> 3a. Time-based tx batching
>>> 	dpif-netdev: Use microsecond granularity.
>>> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)
>>> 	dpif-netdev: Time based output batching.
>>> 	docs: Describe output packet batching in DPDK guide.
>>> 	NEWS: Mark output packet batching support.
>>> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.
>>> 3c. Detailed PMD Performance metrics
>>> 	dpif-netdev: Detailed performance stats for PMDs
>>> 	dpif-netdev: Detection and logging of suspicious PMD iterations
>>>
>>> What do you think?
>>
>> Basically, this looks good to me. But I still think that we should work on that
>> step-by-step not tying to make all the work at once. This will save time of
>> rebasing on intermediate versions of patches.
> 
> Fine with me as long as that doesn't stop review and testing of the not yet rebased
> patches 3a-c. We need those tests and reviews to find and address any deficiencies
> inherent in the feature (independent from rebasing).
> 

I'm not sure if there is some reason you have tied those patches (3)
together. I thought the idea now was to keep things separate?

I'm making a few edits on 3b atm. Can possibly take this out of the
chain. It applies without any of the other patches, but I'm not sure if
it's functional yet.

>> I'll try to spend some time from the rest of today to check out "nestable cycle timers".
>> Would like to see fixed patch from #1 and a proper patch for #2.
> 
> I will try to send out patches for #1 (v6) and #2 this afternoon.
> 
>> Step #3a should not be hard.
>>
> 
> @Ian, Kevin and Billy: Should we anyway have a short Skype chat?
> 

I think we have a plan, let's skip and keep on email.

> Regards, Jan
>
Jan Scheurich Jan. 9, 2018, 1:58 p.m. UTC | #14
> >>> My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one patch

> >> that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as possible.

> >>>

> >>> Here’s my suggestions:

> >>> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf

> >>> 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)

> >>> 3a. Time-based tx batching

> >>> 	dpif-netdev: Use microsecond granularity.

> >>> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)

> >>> 	dpif-netdev: Time based output batching.

> >>> 	docs: Describe output packet batching in DPDK guide.

> >>> 	NEWS: Mark output packet batching support.

> >>> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.

> >>> 3c. Detailed PMD Performance metrics

> >>> 	dpif-netdev: Detailed performance stats for PMDs

> >>> 	dpif-netdev: Detection and logging of suspicious PMD iterations

> >>>

> >>> What do you think?

> >>

> >> Basically, this looks good to me. But I still think that we should work on that

> >> step-by-step not tying to make all the work at once. This will save time of

> >> rebasing on intermediate versions of patches.

> >

> > Fine with me as long as that doesn't stop review and testing of the not yet rebased

> > patches 3a-c. We need those tests and reviews to find and address any deficiencies

> > inherent in the feature (independent from rebasing).

> >

> 

> I'm not sure if there is some reason you have tied those patches (3)

> together. I thought the idea now was to keep things separate?


The idea is to have them as decoupled as possible once the first two refactoring patches are in place. Ideally one could apply them in any order. At least with much less effort than currently and lots of reverting changes. I didn't want to imply any specific order here.

> 

> I'm making a few edits on 3b atm. Can possibly take this out of the

> chain. It applies without any of the other patches, but I'm not sure if

> it's functional yet.


It should apply after refactoring patches #1 and #2. Otherwise we will have even more work to do the refactoring later. Do you work on the simplified version I proposed?

> 

> >> I'll try to spend some time from the rest of today to check out "nestable cycle timers".

> >> Would like to see fixed patch from #1 and a proper patch for #2.

> >

> > I will try to send out patches for #1 (v6) and #2 this afternoon.

> >

> >> Step #3a should not be hard.

> >>

> >

> > @Ian, Kevin and Billy: Should we anyway have a short Skype chat?

> >

> 

> I think we have a plan, let's skip and keep on email.


Fine with me.
Jan Scheurich Jan. 10, 2018, 8:14 a.m. UTC | #15
Hi,

I have just sent out a series with patches #1 and #2 as agreed. They address comments by Ilya on #1 and the draft for nestable cycle counters. 

I haven't done extensive tests yet as I wanted to share as early as possible.
I will continue to test and prepare a rebased v6 of the remainder of the PMD metrics #3c series soon.

Looking forward to review and test rebased #3a and #3b.

BR, Jan

> -----Original Message-----

> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Jan Scheurich

> Sent: Tuesday, 09 January, 2018 14:58

> To: Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>

> Cc: dev@openvswitch.org

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

> 

> > >>> My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one

> patch

> > >> that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as

> possible.

> > >>>

> > >>> Here’s my suggestions:

> > >>> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf

> > >>> 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)

> > >>> 3a. Time-based tx batching

> > >>> 	dpif-netdev: Use microsecond granularity.

> > >>> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)

> > >>> 	dpif-netdev: Time based output batching.

> > >>> 	docs: Describe output packet batching in DPDK guide.

> > >>> 	NEWS: Mark output packet batching support.

> > >>> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.

> > >>> 3c. Detailed PMD Performance metrics

> > >>> 	dpif-netdev: Detailed performance stats for PMDs

> > >>> 	dpif-netdev: Detection and logging of suspicious PMD iterations

> > >>>

> > >>> What do you think?

> > >>

> > >> Basically, this looks good to me. But I still think that we should work on that

> > >> step-by-step not tying to make all the work at once. This will save time of

> > >> rebasing on intermediate versions of patches.

> > >

> > > Fine with me as long as that doesn't stop review and testing of the not yet rebased

> > > patches 3a-c. We need those tests and reviews to find and address any deficiencies

> > > inherent in the feature (independent from rebasing).

> > >

> >

> > I'm not sure if there is some reason you have tied those patches (3)

> > together. I thought the idea now was to keep things separate?

> 

> The idea is to have them as decoupled as possible once the first two refactoring patches are in place. Ideally one could apply them in any

> order. At least with much less effort than currently and lots of reverting changes. I didn't want to imply any specific order here.

> 

> >

> > I'm making a few edits on 3b atm. Can possibly take this out of the

> > chain. It applies without any of the other patches, but I'm not sure if

> > it's functional yet.

> 

> It should apply after refactoring patches #1 and #2. Otherwise we will have even more work to do the refactoring later. Do you work on

> the simplified version I proposed?

> 

> >

> > >> I'll try to spend some time from the rest of today to check out "nestable cycle timers".

> > >> Would like to see fixed patch from #1 and a proper patch for #2.

> > >

> > > I will try to send out patches for #1 (v6) and #2 this afternoon.

> > >

> > >> Step #3a should not be hard.

> > >>

> > >

> > > @Ian, Kevin and Billy: Should we anyway have a short Skype chat?

> > >

> >

> > I think we have a plan, let's skip and keep on email.

> 

> Fine with me.

> _______________________________________________

> dev mailing list

> dev@openvswitch.org

> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kevin Traynor Jan. 11, 2018, 2:40 p.m. UTC | #16
On 01/10/2018 08:14 AM, Jan Scheurich wrote:
> Hi,
> 
> I have just sent out a series with patches #1 and #2 as agreed. They address comments by Ilya on #1 and the draft for nestable cycle counters. 
> 
> I haven't done extensive tests yet as I wanted to share as early as possible.
> I will continue to test and prepare a rebased v6 of the remainder of the PMD metrics #3c series soon.
> 
> Looking forward to review and test rebased #3a and #3b.
> 
> BR, Jan

Hi Jan/all - I just sent a new version of the balance stats (3b) in
reply to the original series. I liked that it was being simplified to
not count the rxq idle cycles in the merged series version but there was
still a lot of code for storage/calculations of % of pmd in the rxqs. It
got me thinking that it could be simplified a lot more by moving these
directly to the pmd, so I did that.

I think it is now pretty independent of the other patch sets that are
being developed around the pmd, so there should be no dependency issues.
Probably at most a trivial rebase if other patches got merged before or
after it.

thanks,
Kevin.

> 
>> -----Original Message-----
>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Jan Scheurich
>> Sent: Tuesday, 09 January, 2018 14:58
>> To: Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>
>> Cc: dev@openvswitch.org
>> Subject: Re: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle count and rebased patches
>>
>>>>>> My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one
>> patch
>>>>> that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as
>> possible.
>>>>>>
>>>>>> Here’s my suggestions:
>>>>>> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf
>>>>>> 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)
>>>>>> 3a. Time-based tx batching
>>>>>> 	dpif-netdev: Use microsecond granularity.
>>>>>> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)
>>>>>> 	dpif-netdev: Time based output batching.
>>>>>> 	docs: Describe output packet batching in DPDK guide.
>>>>>> 	NEWS: Mark output packet batching support.
>>>>>> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.
>>>>>> 3c. Detailed PMD Performance metrics
>>>>>> 	dpif-netdev: Detailed performance stats for PMDs
>>>>>> 	dpif-netdev: Detection and logging of suspicious PMD iterations
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Basically, this looks good to me. But I still think that we should work on that
>>>>> step-by-step not tying to make all the work at once. This will save time of
>>>>> rebasing on intermediate versions of patches.
>>>>
>>>> Fine with me as long as that doesn't stop review and testing of the not yet rebased
>>>> patches 3a-c. We need those tests and reviews to find and address any deficiencies
>>>> inherent in the feature (independent from rebasing).
>>>>
>>>
>>> I'm not sure if there is some reason you have tied those patches (3)
>>> together. I thought the idea now was to keep things separate?
>>
>> The idea is to have them as decoupled as possible once the first two refactoring patches are in place. Ideally one could apply them in any
>> order. At least with much less effort than currently and lots of reverting changes. I didn't want to imply any specific order here.
>>
>>>
>>> I'm making a few edits on 3b atm. Can possibly take this out of the
>>> chain. It applies without any of the other patches, but I'm not sure if
>>> it's functional yet.
>>
>> It should apply after refactoring patches #1 and #2. Otherwise we will have even more work to do the refactoring later. Do you work on
>> the simplified version I proposed?
>>
>>>
>>>>> I'll try to spend some time from the rest of today to check out "nestable cycle timers".
>>>>> Would like to see fixed patch from #1 and a proper patch for #2.
>>>>
>>>> I will try to send out patches for #1 (v6) and #2 this afternoon.
>>>>
>>>>> Step #3a should not be hard.
>>>>>
>>>>
>>>> @Ian, Kevin and Billy: Should we anyway have a short Skype chat?
>>>>
>>>
>>> I think we have a plan, let's skip and keep on email.
>>
>> Fine with me.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian Jan. 11, 2018, 2:58 p.m. UTC | #17
> On 01/10/2018 08:14 AM, Jan Scheurich wrote:

> > Hi,

> >

> > I have just sent out a series with patches #1 and #2 as agreed. They

> address comments by Ilya on #1 and the draft for nestable cycle counters.

> >

> > I haven't done extensive tests yet as I wanted to share as early as

> possible.

> > I will continue to test and prepare a rebased v6 of the remainder of the

> PMD metrics #3c series soon.

> >

> > Looking forward to review and test rebased #3a and #3b.

> >

> > BR, Jan

> 

> Hi Jan/all - I just sent a new version of the balance stats (3b) in reply

> to the original series. I liked that it was being simplified to not count

> the rxq idle cycles in the merged series version but there was still a lot

> of code for storage/calculations of % of pmd in the rxqs. It got me

> thinking that it could be simplified a lot more by moving these directly

> to the pmd, so I did that.

> 

> I think it is now pretty independent of the other patch sets that are

> being developed around the pmd, so there should be no dependency issues.

> Probably at most a trivial rebase if other patches got merged before or

> after it.


+1, I think this set looks fairly independent now, I'll have some spare cycles this evening to take a look.

Ian
> 

> thanks,

> Kevin.

> 

> >

> >> -----Original Message-----

> >> From: ovs-dev-bounces@openvswitch.org

> >> [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Jan Scheurich

> >> Sent: Tuesday, 09 January, 2018 14:58

> >> To: Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets

> >> <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>

> >> Cc: dev@openvswitch.org

> >> Subject: Re: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle

> >> count and rebased patches

> >>

> >>>>>> My suggestion would be to start with the least controversial

> >>>>>> refactoring first so that we do not introduce complex things in

> >>>>>> one

> >> patch

> >>>>> that we then throw out in the next one again. By that let's try to

> >>>>> make the actual feature patches as small and independent as

> >> possible.

> >>>>>>

> >>>>>> Here’s my suggestions:

> >>>>>> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf 2.

> >>>>>> dpif-netdev: Refactor cycle counting (nestable cycle timer) 3a.

> >>>>>> Time-based tx batching

> >>>>>> 	dpif-netdev: Use microsecond granularity.

> >>>>>> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable

> cycle timers)

> >>>>>> 	dpif-netdev: Time based output batching.

> >>>>>> 	docs: Describe output packet batching in DPDK guide.

> >>>>>> 	NEWS: Mark output packet batching support.

> >>>>>> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.

> >>>>>> 3c. Detailed PMD Performance metrics

> >>>>>> 	dpif-netdev: Detailed performance stats for PMDs

> >>>>>> 	dpif-netdev: Detection and logging of suspicious PMD iterations

> >>>>>>

> >>>>>> What do you think?

> >>>>>

> >>>>> Basically, this looks good to me. But I still think that we should

> >>>>> work on that step-by-step not tying to make all the work at once.

> >>>>> This will save time of rebasing on intermediate versions of patches.

> >>>>

> >>>> Fine with me as long as that doesn't stop review and testing of the

> >>>> not yet rebased patches 3a-c. We need those tests and reviews to

> >>>> find and address any deficiencies inherent in the feature

> (independent from rebasing).

> >>>>

> >>>

> >>> I'm not sure if there is some reason you have tied those patches (3)

> >>> together. I thought the idea now was to keep things separate?

> >>

> >> The idea is to have them as decoupled as possible once the first two

> >> refactoring patches are in place. Ideally one could apply them in any

> order. At least with much less effort than currently and lots of reverting

> changes. I didn't want to imply any specific order here.

> >>

> >>>

> >>> I'm making a few edits on 3b atm. Can possibly take this out of the

> >>> chain. It applies without any of the other patches, but I'm not sure

> >>> if it's functional yet.

> >>

> >> It should apply after refactoring patches #1 and #2. Otherwise we

> >> will have even more work to do the refactoring later. Do you work on

> the simplified version I proposed?

> >>

> >>>

> >>>>> I'll try to spend some time from the rest of today to check out

> "nestable cycle timers".

> >>>>> Would like to see fixed patch from #1 and a proper patch for #2.

> >>>>

> >>>> I will try to send out patches for #1 (v6) and #2 this afternoon.

> >>>>

> >>>>> Step #3a should not be hard.

> >>>>>

> >>>>

> >>>> @Ian, Kevin and Billy: Should we anyway have a short Skype chat?

> >>>>

> >>>

> >>> I think we have a plan, let's skip and keep on email.

> >>

> >> Fine with me.

> >> _______________________________________________

> >> dev mailing list

> >> dev@openvswitch.org

> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Jan. 12, 2018, 11:26 a.m. UTC | #18
On 10.01.2018 11:14, Jan Scheurich wrote:
> Hi,
> 
> I have just sent out a series with patches #1 and #2 as agreed. They address comments by Ilya on #1 and the draft for nestable cycle counters. 
> 
> I haven't done extensive tests yet as I wanted to share as early as possible.
> I will continue to test and prepare a rebased v6 of the remainder of the PMD metrics #3c series soon.
> 
> Looking forward to review and test rebased #3a and #3b.

I just rebased time based output batching (3a) on top of
'[PATCH v8 0/2] Refactor PMD stats and cycle counting'.

New v10 version available here:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=22919

v8 of PMD stats refactoring looks close to be ready for merging.
Just few more fixes required with arm build and pmd_stats_init.
I'll continue to review and test.

Best regards, Ilya Maximets.

> 
> BR, Jan
> 
>> -----Original Message-----
>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Jan Scheurich
>> Sent: Tuesday, 09 January, 2018 14:58
>> To: Kevin Traynor <ktraynor@redhat.com>; Ilya Maximets <i.maximets@samsung.com>; Stokes, Ian <ian.stokes@intel.com>
>> Cc: dev@openvswitch.org
>> Subject: Re: [ovs-dev] [RFC PATCH 0/8] dpif-netdev: Refactor cycle count and rebased patches
>>
>>>>>> My suggestion would be to start with the least controversial refactoring first so that we do not introduce complex things in one
>> patch
>>>>> that we then throw out in the next one again. By that let's try to make the actual feature patches as small and independent as
>> possible.
>>>>>>
>>>>>> Here’s my suggestions:
>>>>>> 1. dpif-netdev: Refactor PMD performance into dpif-netdev-perf
>>>>>> 2. dpif-netdev: Refactor cycle counting (nestable cycle timer)
>>>>>> 3a. Time-based tx batching
>>>>>> 	dpif-netdev: Use microsecond granularity.
>>>>>> 	dpif-netdev: Count cycles on per-rxq basis. (using the nestable cycle timers)
>>>>>> 	dpif-netdev: Time based output batching.
>>>>>> 	docs: Describe output packet batching in DPDK guide.
>>>>>> 	NEWS: Mark output packet batching support.
>>>>>> 3b. dpif-netdev: Add percentage of pmd/core used by each rxq.
>>>>>> 3c. Detailed PMD Performance metrics
>>>>>> 	dpif-netdev: Detailed performance stats for PMDs
>>>>>> 	dpif-netdev: Detection and logging of suspicious PMD iterations
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Basically, this looks good to me. But I still think that we should work on that
>>>>> step-by-step not tying to make all the work at once. This will save time of
>>>>> rebasing on intermediate versions of patches.
>>>>
>>>> Fine with me as long as that doesn't stop review and testing of the not yet rebased
>>>> patches 3a-c. We need those tests and reviews to find and address any deficiencies
>>>> inherent in the feature (independent from rebasing).
>>>>
>>>
>>> I'm not sure if there is some reason you have tied those patches (3)
>>> together. I thought the idea now was to keep things separate?
>>
>> The idea is to have them as decoupled as possible once the first two refactoring patches are in place. Ideally one could apply them in any
>> order. At least with much less effort than currently and lots of reverting changes. I didn't want to imply any specific order here.
>>
>>>
>>> I'm making a few edits on 3b atm. Can possibly take this out of the
>>> chain. It applies without any of the other patches, but I'm not sure if
>>> it's functional yet.
>>
>> It should apply after refactoring patches #1 and #2. Otherwise we will have even more work to do the refactoring later. Do you work on
>> the simplified version I proposed?
>>
>>>
>>>>> I'll try to spend some time from the rest of today to check out "nestable cycle timers".
>>>>> Would like to see fixed patch from #1 and a proper patch for #2.
>>>>
>>>> I will try to send out patches for #1 (v6) and #2 this afternoon.
>>>>
>>>>> Step #3a should not be hard.
>>>>>
>>>>
>>>> @Ian, Kevin and Billy: Should we anyway have a short Skype chat?
>>>>
>>>
>>> I think we have a plan, let's skip and keep on email.
>>
>> Fine with me.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stokes, Ian Jan. 12, 2018, 11:52 a.m. UTC | #19
> On 10.01.2018 11:14, Jan Scheurich wrote:
> > Hi,
> >
> > I have just sent out a series with patches #1 and #2 as agreed. They
> address comments by Ilya on #1 and the draft for nestable cycle counters.
> >
> > I haven't done extensive tests yet as I wanted to share as early as
> possible.
> > I will continue to test and prepare a rebased v6 of the remainder of the
> PMD metrics #3c series soon.
> >
> > Looking forward to review and test rebased #3a and #3b.
> 
> I just rebased time based output batching (3a) on top of '[PATCH v8 0/2]
> Refactor PMD stats and cycle counting'.
> 
> New v10 version available here:
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=22919
> 
> v8 of PMD stats refactoring looks close to be ready for merging.
> Just few more fixes required with arm build and pmd_stats_init.
> I'll continue to review and test.
> 
> Best regards, Ilya Maximets.

Thanks for this Ilya, the series is starting to take shape now. I can start taking a look at this and Kevin's PMD patchset today.

Thanks
Ian