| Message ID | 20220105165349.3447695-1-emma.finn@intel.com |
|---|---|
| Headers | show |
| Series | Actions Infrastructure + Optimizations | expand |
> -----Original Message----- > From: Finn, Emma <emma.finn@intel.com> > Sent: Wednesday, January 5, 2022 4:54 PM > To: dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>; > Amber, Kumar <kumar.amber@intel.com> > Cc: Finn, Emma <emma.finn@intel.com> > Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > --- > v4: > - Rebase to master > - Add ISA implementation of push_vlan action Thanks for the updated patchset Emma & Amber. Overall, this is working as expected and I've only had some minor comments throughout the patchset. I've added my Acked-by to most patches, some small open questions remain to be addressed in a v5. +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work towards that. Regards, -Harry
On 1/6/22 14:11, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Finn, Emma <emma.finn@intel.com> >> Sent: Wednesday, January 5, 2022 4:54 PM >> To: dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>; >> Amber, Kumar <kumar.amber@intel.com> >> Cc: Finn, Emma <emma.finn@intel.com> >> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> --- >> v4: >> - Rebase to master >> - Add ISA implementation of push_vlan action > > Thanks for the updated patchset Emma & Amber. > > Overall, this is working as expected and I've only had some minor > comments throughout the patchset. I've added my Acked-by to most > patches, some small open questions remain to be addressed in a v5. > > +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work towards that. Hi, Harry, Ian, others. Following up from a brief conversation during today's upstream meeting. It was brought to my attention that you're expecting this series and the 'hash' one to be accepted into 2.17. Though there are few issues with that: 1. This review for v4 was actually very first review of the patch set. The other one as of today doesn't have any reviews at all. Looking at the change log for this patch set it doesn't seem that internal reviews behind the closed doors (if there were any) requested any significant changes. In any case, internal reviews is not the way how open-source projects work. 2. The soft freeze for 2.17 began on Jan 3 in accordance with our release schedule (even a bit later), and as you know, during the soft freeze we're not normally accepting patches that wasn't already reviewed before the soft freeze begun. https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html That's not the end of a world, but you need to request an exception in reply to the email linked above. But I have a few high-level concerns regarding the patch set itself, and that's a bigger problem for me: 1. What are the benefits of these patch sets? A lot of infrastructure changes are made, but the benefits of them are unclear. Why these changes are needed in the end? I believe, that was the main reason why community had no interest in reviewing these patches. 2.17 is supposed to be a new LTS, so infrastructure changes without clear benefits might not be a good fit taking into account time constraints and lack of reviews. 2. The same concern that I already brought to you attention in other conversations, e.g. on the ovs-security list about a month ago. It's related to all developments in that area: why this is tied to the userspace datapath? i.e. why execution of actions depends on the datapath? This seems artificial and complicates testing a lot. Like current autovalidator is not able to test most of the packet parsing cases, the same way autovalidator will not be able to test execution of actions. I have some more comments, but they are more related to the actual code and above 2 are the most important for now. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Wednesday, January 12, 2022 6:01 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > <kumar.amber@intel.com> > Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com> > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/6/22 14:11, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Finn, Emma <emma.finn@intel.com> > >> Sent: Wednesday, January 5, 2022 4:54 PM > >> To: dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>; > >> Amber, Kumar <kumar.amber@intel.com> > >> Cc: Finn, Emma <emma.finn@intel.com> > >> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> --- > >> v4: > >> - Rebase to master > >> - Add ISA implementation of push_vlan action > > > > Thanks for the updated patchset Emma & Amber. > > > > Overall, this is working as expected and I've only had some minor > > comments throughout the patchset. I've added my Acked-by to most > > patches, some small open questions remain to be addressed in a v5. > > > > +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work > towards that. > > Hi, Harry, Ian, others. Hi Ilya, > Following up from a brief conversation during today's upstream meeting. > It was brought to my attention that you're expecting this series and > the 'hash' one to be accepted into 2.17. Though there are few issues > with that: > > 1. This review for v4 was actually very first review of the patch set. > The other one as of today doesn't have any reviews at all. > Looking at the change log for this patch set it doesn't seem that > internal reviews behind the closed doors (if there were any) requested > any significant changes. In any case, internal reviews is not the way > how open-source projects work. Actions & MFEX were developed internally yes, and hence internal reviews and architecture was iterated on. Saying "not many large changes requested" is not relevant, it means that internally the architecture was well aligned. If anything, it means that reviewers did not have big concerns, hence we should have better confidence to merge. > 2. The soft freeze for 2.17 began on Jan 3 in accordance with our > release schedule (even a bit later), and as you know, during the soft > freeze we're not normally accepting patches that wasn't already reviewed > before the soft freeze begun. > https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html Actions and MFEX Hashing were discussed and reviewed in public at OVS Conf; https://www.openvswitch.org/support/ovscon2021/#T32 https://www.openvswitch.org/support/ovscon2021/#T33 The closing "call to action" slide clearly states 2.17 upstream is intended, and welcome community review & comments, no concerns were raised. > That's not the end of a world, but you need to request an exception in > reply to the email linked above. As both the patches and intent to merge for OVS 2.17 were clearly discussed in public at OVS Conf, this "request exception" case does not apply. > But I have a few high-level concerns regarding the patch set itself, > and that's a bigger problem for me: > > 1. What are the benefits of these patch sets? A lot of infrastructure > changes are made, but the benefits of them are unclear. Why these > changes are needed in the end? I believe, that was the main reason > why community had no interest in reviewing these patches. > 2.17 is supposed to be a new LTS, so infrastructure changes without > clear benefits might not be a good fit taking into account time > constraints and lack of reviews. Customers workloads are accelerated, improving OVS datapath performance for their workloads. The customers are not active in SW development themselves, hence there is no reviews from them on OVS ML. If you have concerns over a patchset, these should be raised against a V1 of a patchset, in appropriate time to be addressed. Asking these questions the week of the integration deadline is unacceptable, and may not be used to block merge a patchset. > 2. The same concern that I already brought to you attention in other > conversations, e.g. on the ovs-security list about a month ago. > It's related to all developments in that area: why this is tied to > the userspace datapath? i.e. why execution of actions depends on the > datapath? This seems artificial and complicates testing a lot. > Like current autovalidator is not able to test most of the packet > parsing cases, the same way autovalidator will not be able to test > execution of actions. Regarding testing, as per that same conversation, I replied that yes we would investigate improving testing even further. There is ongoing work here around improving our fuzz testing. Autovalidation is not complex, it validates all unit-tests, and has been presented at OVS conferences, and upstreamed for multiple components (DPCLS, MFEX, and now Actions). If there is concern over approach, then this should not be held against Actions patchset now in the week of merge, but addressed in general. Not acceptable to block Actions or Hashing patchsets based on this, in the week of the integration deadline. > I have some more comments, but they are more related to the actual code > and above 2 are the most important for now. > > Best regards, Ilya Maximets. Regards, -Harry
Hi, On Thu, Jan 13, 2022 at 10:53:57AM +0000, Van Haaren, Harry wrote: > > -----Original Message----- > > From: Ilya Maximets <i.maximets@ovn.org> > > Sent: Wednesday, January 12, 2022 6:01 PM > > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > > <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > > <kumar.amber@intel.com> > > Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > > <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com> > > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > > > On 1/6/22 14:11, Van Haaren, Harry wrote: > > >> -----Original Message----- > > >> From: Finn, Emma <emma.finn@intel.com> > > >> Sent: Wednesday, January 5, 2022 4:54 PM > > >> To: dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>; > > >> Amber, Kumar <kumar.amber@intel.com> > > >> Cc: Finn, Emma <emma.finn@intel.com> > > >> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > >> > > >> --- > > >> v4: > > >> - Rebase to master > > >> - Add ISA implementation of push_vlan action > > > > > > Thanks for the updated patchset Emma & Amber. > > > > > > Overall, this is working as expected and I've only had some minor > > > comments throughout the patchset. I've added my Acked-by to most > > > patches, some small open questions remain to be addressed in a v5. > > > > > > +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work > > towards that. > > > > Hi, Harry, Ian, others. > > Hi Ilya, > > > Following up from a brief conversation during today's upstream meeting. > > It was brought to my attention that you're expecting this series and > > the 'hash' one to be accepted into 2.17. Though there are few issues > > with that: > > > > 1. This review for v4 was actually very first review of the patch set. > > The other one as of today doesn't have any reviews at all. > > Looking at the change log for this patch set it doesn't seem that > > internal reviews behind the closed doors (if there were any) requested > > any significant changes. In any case, internal reviews is not the way > > how open-source projects work. > > Actions & MFEX were developed internally yes, and hence internal reviews and > architecture was iterated on. Saying "not many large changes requested" is not relevant, > it means that internally the architecture was well aligned. If anything, it means that > reviewers did not have big concerns, hence we should have better confidence to merge. Yes but it can also mean that no careful review has been done. That's why the ask here is to do the review in the open, otherwise others can't tell what happened. > > 2. The soft freeze for 2.17 began on Jan 3 in accordance with our > > release schedule (even a bit later), and as you know, during the soft > > freeze we're not normally accepting patches that wasn't already reviewed > > before the soft freeze begun. > > https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html > > Actions and MFEX Hashing were discussed and reviewed in public at OVS Conf; > https://www.openvswitch.org/support/ovscon2021/#T32 > https://www.openvswitch.org/support/ovscon2021/#T33 > > The closing "call to action" slide clearly states 2.17 upstream is intended, > and welcome community review & comments, no concerns were raised. There is a difference between a talk and pushing patches, and more importantly, to get the community to review and ACK the patches. > > That's not the end of a world, but you need to request an exception in > > reply to the email linked above. > > As both the patches and intent to merge for OVS 2.17 were clearly discussed > in public at OVS Conf, this "request exception" case does not apply. The talk happened at Dec 7 & 8, the soft freeze happened Jan 3rd, so there was a chunk of time to work on that. I don't see why one thing justifies the other. > > But I have a few high-level concerns regarding the patch set itself, > > and that's a bigger problem for me: > > > > 1. What are the benefits of these patch sets? A lot of infrastructure > > changes are made, but the benefits of them are unclear. Why these > > changes are needed in the end? I believe, that was the main reason > > why community had no interest in reviewing these patches. > > 2.17 is supposed to be a new LTS, so infrastructure changes without > > clear benefits might not be a good fit taking into account time > > constraints and lack of reviews. > > Customers workloads are accelerated, improving OVS datapath performance for their > workloads. The customers are not active in SW development themselves, hence there > is no reviews from them on OVS ML. Can you back up that with numbers? Forgive me if I lost something, but I have no idea on how much it helps to justify the changes and extra complexity. Best Regards, fbl > If you have concerns over a patchset, these should be raised against a V1 of a patchset, > in appropriate time to be addressed. Asking these questions the week of the integration > deadline is unacceptable, and may not be used to block merge a patchset. > > > > 2. The same concern that I already brought to you attention in other > > conversations, e.g. on the ovs-security list about a month ago. > > It's related to all developments in that area: why this is tied to > > the userspace datapath? i.e. why execution of actions depends on the > > datapath? This seems artificial and complicates testing a lot. > > Like current autovalidator is not able to test most of the packet > > parsing cases, the same way autovalidator will not be able to test > > execution of actions. > > Regarding testing, as per that same conversation, I replied that yes we would investigate > improving testing even further. There is ongoing work here around improving our fuzz testing. > > Autovalidation is not complex, it validates all unit-tests, and has been presented at OVS > conferences, and upstreamed for multiple components (DPCLS, MFEX, and now Actions). > If there is concern over approach, then this should not be held against Actions patchset > now in the week of merge, but addressed in general. Not acceptable to block Actions > or Hashing patchsets based on this, in the week of the integration deadline. > > > > I have some more comments, but they are more related to the actual code > > and above 2 are the most important for now. > > > > Best regards, Ilya Maximets. > > Regards, -Harry > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1/13/22 11:53, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Wednesday, January 12, 2022 6:01 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma >> <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar >> <kumar.amber@intel.com> >> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner >> <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com> >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> On 1/6/22 14:11, Van Haaren, Harry wrote: >>>> -----Original Message----- >>>> From: Finn, Emma <emma.finn@intel.com> >>>> Sent: Wednesday, January 5, 2022 4:54 PM >>>> To: dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com>; >>>> Amber, Kumar <kumar.amber@intel.com> >>>> Cc: Finn, Emma <emma.finn@intel.com> >>>> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations >>>> >>>> --- >>>> v4: >>>> - Rebase to master >>>> - Add ISA implementation of push_vlan action >>> >>> Thanks for the updated patchset Emma & Amber. >>> >>> Overall, this is working as expected and I've only had some minor >>> comments throughout the patchset. I've added my Acked-by to most >>> patches, some small open questions remain to be addressed in a v5. >>> >>> +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work >> towards that. >> >> Hi, Harry, Ian, others. > > Hi Ilya, > >> Following up from a brief conversation during today's upstream meeting. >> It was brought to my attention that you're expecting this series and >> the 'hash' one to be accepted into 2.17. Though there are few issues >> with that: >> >> 1. This review for v4 was actually very first review of the patch set. >> The other one as of today doesn't have any reviews at all. >> Looking at the change log for this patch set it doesn't seem that >> internal reviews behind the closed doors (if there were any) requested >> any significant changes. In any case, internal reviews is not the way >> how open-source projects work. > > Actions & MFEX were developed internally yes, and hence internal reviews and > architecture was iterated on. Saying "not many large changes requested" is not relevant, > it means that internally the architecture was well aligned. If anything, it means that > reviewers did not have big concerns, hence we should have better confidence to merge. Me, who never seen these reviews and architecture discussions has exactly zero confidence in these patch-set the same as the rest of the community. Development behind closed doors is never taken into account while making decisions in open-source projects, because this development and made decisions are not visible and can not be peer-reviewed by anyone outside. On a quick glance over the code and reviews done in a last couple of days, I'd also question the quality of these public reviews as the patch set at least contains a few issues in a copy-pasted code that was already fixed on master branch (left alone that these code duplications should, likley, not exist in a first place). > > >> 2. The soft freeze for 2.17 began on Jan 3 in accordance with our >> release schedule (even a bit later), and as you know, during the soft >> freeze we're not normally accepting patches that wasn't already reviewed >> before the soft freeze begun. >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html > > Actions and MFEX Hashing were discussed and reviewed in public at OVS Conf; > https://www.openvswitch.org/support/ovscon2021/#T32 > https://www.openvswitch.org/support/ovscon2021/#T33 They were presented, but not discussed or reviewed. > > The closing "call to action" slide clearly states 2.17 upstream is intended, > and welcome community review & comments, no concerns were raised. > >> That's not the end of a world, but you need to request an exception in >> reply to the email linked above. > > As both the patches and intent to merge for OVS 2.17 were clearly discussed > in public at OVS Conf, this "request exception" case does not apply. Again, they were presented, not discussed. It's the same as sending patches to the mailing list. We don't have a policy to accept everything that wasn't explicitly rejected. Lack of feedback / responses in most cases means lack of interest. > > >> But I have a few high-level concerns regarding the patch set itself, >> and that's a bigger problem for me: >> >> 1. What are the benefits of these patch sets? A lot of infrastructure >> changes are made, but the benefits of them are unclear. Why these >> changes are needed in the end? I believe, that was the main reason >> why community had no interest in reviewing these patches. >> 2.17 is supposed to be a new LTS, so infrastructure changes without >> clear benefits might not be a good fit taking into account time >> constraints and lack of reviews. > > Customers workloads are accelerated, improving OVS datapath performance for their > workloads. The customers are not active in SW development themselves, hence there > is no reviews from them on OVS ML. I don't see anything being accelerated. I'm not asking your customers to prove anything, I'm asking you and your team to provide a reason to review the patches. "Workloads are accelerated" is a blunt statement without any numbers. And even that you're saying for a very first time. For what I know it could be 0.00001% improvement that simply doesn't worth it. I don't see any claims about performance regression testing being made either. > > If you have concerns over a patchset, these should be raised against a V1 of a patchset, I don't have time to look at every patch-set that doesn't even try to make me or anyone else interested in reviewing it and you're re-spinning the series without any reviews being made, so that is simply not practical. > in appropriate time to be addressed. Asking these questions the week of the integration > deadline is unacceptable, and may not be used to block merge a patchset. Again, there is no policy to accept everything that wasn't explicitly rejected. You missed the review deadline for a soft freeze. So, these patches was not considered for integration at all at this point. Hence questioning them is justified. > > >> 2. The same concern that I already brought to you attention in other >> conversations, e.g. on the ovs-security list about a month ago. >> It's related to all developments in that area: why this is tied to >> the userspace datapath? i.e. why execution of actions depends on the >> datapath? This seems artificial and complicates testing a lot. >> Like current autovalidator is not able to test most of the packet >> parsing cases, the same way autovalidator will not be able to test >> execution of actions. > > Regarding testing, as per that same conversation, I replied that yes we would investigate > improving testing even further. There is ongoing work here around improving our fuzz testing. Didn't see any progress from your side on that front. And still waiting for your reply on this email: https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/389945.html > > Autovalidation is not complex, it validates all unit-tests, and has been presented at OVS 'all unit tests' is a plain lie here. You're perfectly aware that this is not true at least for miniflow_extract part. > conferences, and upstreamed for multiple components (DPCLS, MFEX, and now Actions). > If there is concern over approach, then this should not be held against Actions patchset > now in the week of merge, but addressed in general. AFAICT, you're not planning to address concerns in general, because the issue with miniflow_extract autovalidation not being usable for a big chunk of existing tests was flagged several times including the email mentioned above, and you're always avoiding to reply to these concerns. > Not acceptable to block Actions > or Hashing patchsets based on this, in the week of the integration deadline. Continuing to re-use and build new things on top of broken abstractions doesn't look good to me. > > >> I have some more comments, but they are more related to the actual code >> and above 2 are the most important for now. >> >> Best regards, Ilya Maximets. > > Regards, -Harry >
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Thursday, January 13, 2022 1:14 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > <kumar.amber@intel.com> > Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com>; Mcnamara, John > <john.mcnamara@intel.com> > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/13/22 11:53, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Wednesday, January 12, 2022 6:01 PM > >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > >> <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > >> <kumar.amber@intel.com> > >> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > >> <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com> > >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> On 1/6/22 14:11, Van Haaren, Harry wrote: > >>>> -----Original Message----- > >>>> From: Finn, Emma <emma.finn@intel.com> > >>>> Sent: Wednesday, January 5, 2022 4:54 PM > >>>> To: dev@openvswitch.org; Van Haaren, Harry > <harry.van.haaren@intel.com>; > >>>> Amber, Kumar <kumar.amber@intel.com> > >>>> Cc: Finn, Emma <emma.finn@intel.com> > >>>> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >>>> > >>>> --- > >>>> v4: > >>>> - Rebase to master > >>>> - Add ISA implementation of push_vlan action > >>> > >>> Thanks for the updated patchset Emma & Amber. > >>> > >>> Overall, this is working as expected and I've only had some minor > >>> comments throughout the patchset. I've added my Acked-by to most > >>> patches, some small open questions remain to be addressed in a v5. > >>> > >>> +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work > >> towards that. > >> > >> Hi, Harry, Ian, others. > > > > Hi Ilya, > > > >> Following up from a brief conversation during today's upstream meeting. > >> It was brought to my attention that you're expecting this series and > >> the 'hash' one to be accepted into 2.17. Though there are few issues > >> with that: > >> > >> 1. This review for v4 was actually very first review of the patch set. > >> The other one as of today doesn't have any reviews at all. > >> Looking at the change log for this patch set it doesn't seem that > >> internal reviews behind the closed doors (if there were any) requested > >> any significant changes. In any case, internal reviews is not the way > >> how open-source projects work. > > > > Actions & MFEX were developed internally yes, and hence internal reviews and > > architecture was iterated on. Saying "not many large changes requested" is not > relevant, > > it means that internally the architecture was well aligned. If anything, it means > that > > reviewers did not have big concerns, hence we should have better confidence > to merge. > > Me, who never seen these reviews and architecture discussions has exactly > zero confidence in these patch-set the same as the rest of the community. > Development behind closed doors is never taken into account while making > decisions in open-source projects, because this development and made > decisions are not visible and can not be peer-reviewed by anyone outside. > > On a quick glance over the code and reviews done in a last couple of days, > I'd also question the quality of these public reviews as the patch set > at least contains a few issues in a copy-pasted code that was already > fixed on master branch (left alone that these code duplications should, > likley, not exist in a first place). > > > > > > >> 2. The soft freeze for 2.17 began on Jan 3 in accordance with our > >> release schedule (even a bit later), and as you know, during the soft > >> freeze we're not normally accepting patches that wasn't already reviewed > >> before the soft freeze begun. > >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html > > > > Actions and MFEX Hashing were discussed and reviewed in public at OVS Conf; > > https://www.openvswitch.org/support/ovscon2021/#T32 > > https://www.openvswitch.org/support/ovscon2021/#T33 > > They were presented, but not discussed or reviewed. > > > > > The closing "call to action" slide clearly states 2.17 upstream is intended, > > and welcome community review & comments, no concerns were raised. > > > >> That's not the end of a world, but you need to request an exception in > >> reply to the email linked above. > > > > As both the patches and intent to merge for OVS 2.17 were clearly discussed > > in public at OVS Conf, this "request exception" case does not apply. > > Again, they were presented, not discussed. It's the same as sending patches > to the mailing list. We don't have a policy to accept everything that > wasn't explicitly rejected. Lack of feedback / responses in most cases means > lack of interest. > > >> But I have a few high-level concerns regarding the patch set itself, > >> and that's a bigger problem for me: > >> > >> 1. What are the benefits of these patch sets? A lot of infrastructure > >> changes are made, but the benefits of them are unclear. Why these > >> changes are needed in the end? I believe, that was the main reason > >> why community had no interest in reviewing these patches. > >> 2.17 is supposed to be a new LTS, so infrastructure changes without > >> clear benefits might not be a good fit taking into account time > >> constraints and lack of reviews. > > > > Customers workloads are accelerated, improving OVS datapath performance > for their > > workloads. The customers are not active in SW development themselves, > hence there > > is no reviews from them on OVS ML. > > I don't see anything being accelerated. I'm not asking your customers to > prove anything, I'm asking you and your team to provide a reason to review > the patches. "Workloads are accelerated" is a blunt statement without > any numbers. And even that you're saying for a very first time. For what I > know it could be 0.00001% improvement that simply doesn't worth it. I don't > see any claims about performance regression testing being made either. > > > > > If you have concerns over a patchset, these should be raised against a V1 of a > patchset, > > I don't have time to look at every patch-set that doesn't even try to make > me or anyone else interested in reviewing it and you're re-spinning the > series without any reviews being made, so that is simply not practical. > > > in appropriate time to be addressed. Asking these questions the week of the > integration > > deadline is unacceptable, and may not be used to block merge a patchset. > > Again, there is no policy to accept everything that wasn't explicitly rejected. > You missed the review deadline for a soft freeze. So, these patches was > not considered for integration at all at this point. Hence questioning them > is justified. > > > > > > >> 2. The same concern that I already brought to you attention in other > >> conversations, e.g. on the ovs-security list about a month ago. > >> It's related to all developments in that area: why this is tied to > >> the userspace datapath? i.e. why execution of actions depends on the > >> datapath? This seems artificial and complicates testing a lot. > >> Like current autovalidator is not able to test most of the packet > >> parsing cases, the same way autovalidator will not be able to test > >> execution of actions. > > > > Regarding testing, as per that same conversation, I replied that yes we would > investigate > > improving testing even further. There is ongoing work here around improving > our fuzz testing. > > Didn't see any progress from your side on that front. And still waiting for > your reply on this email: > https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/389945.html > > > > > Autovalidation is not complex, it validates all unit-tests, and has been > presented at OVS > > 'all unit tests' is a plain lie here. You're perfectly aware that this is > not true at least for miniflow_extract part. Please avoid confronting language such as "is a plain lie" and suggesting that I'm purposely trying to mislead, that is not my intent. I expect all OVS community members to be respectful, and especially the OVS Maintainers. The above comments have no place in an open-source community such as OVS. If there is a misunderstanding around the above statement, ask for more explanation, and we can discuss it in a professional and polite manner. > > conferences, and upstreamed for multiple components (DPCLS, MFEX, and > now Actions). > > If there is concern over approach, then this should not be held against Actions > patchset > > now in the week of merge, but addressed in general. > > AFAICT, you're not planning to address concerns in general, because the issue > with miniflow_extract autovalidation not being usable for a big chunk of existing > tests was flagged several times including the email mentioned above, and you're > always avoiding to reply to these concerns. > > > Not acceptable to block Actions > > or Hashing patchsets based on this, in the week of the integration deadline. > > Continuing to re-use and build new things on top of broken abstractions > doesn't look good to me. > > > > > > >> I have some more comments, but they are more related to the actual code > >> and above 2 are the most important for now. > >> > >> Best regards, Ilya Maximets. > > > > Regards, -Harry This patchset has received significant push-back, both on this thread and in Flavio's reply here; https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390732.html I feel that there needs to be a clearer path to success in getting contributions merged upstream in OVS. In particular, concerns need to be raised earlier and not in the last week before merge deadline. On closing this thread for 2.17, please note we will continue to upstream these Actions optimizations for OVS 2.18, and request constructive and actionable feedback in order to achieve any rework required for merging. Regards, -Harry
On 1/14/22 18:11, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Thursday, January 13, 2022 1:14 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma >> <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar >> <kumar.amber@intel.com> >> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner >> <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com>; Mcnamara, John >> <john.mcnamara@intel.com> >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> On 1/13/22 11:53, Van Haaren, Harry wrote: >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@ovn.org> >>>> Sent: Wednesday, January 12, 2022 6:01 PM >>>> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma >>>> <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar >>>> <kumar.amber@intel.com> >>>> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner >>>> <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com> >>>> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >>>> >>>> On 1/6/22 14:11, Van Haaren, Harry wrote: >>>>>> -----Original Message----- >>>>>> From: Finn, Emma <emma.finn@intel.com> >>>>>> Sent: Wednesday, January 5, 2022 4:54 PM >>>>>> To: dev@openvswitch.org; Van Haaren, Harry >> <harry.van.haaren@intel.com>; >>>>>> Amber, Kumar <kumar.amber@intel.com> >>>>>> Cc: Finn, Emma <emma.finn@intel.com> >>>>>> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations >>>>>> >>>>>> --- >>>>>> v4: >>>>>> - Rebase to master >>>>>> - Add ISA implementation of push_vlan action >>>>> >>>>> Thanks for the updated patchset Emma & Amber. >>>>> >>>>> Overall, this is working as expected and I've only had some minor >>>>> comments throughout the patchset. I've added my Acked-by to most >>>>> patches, some small open questions remain to be addressed in a v5. >>>>> >>>>> +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work >>>> towards that. >>>> >>>> Hi, Harry, Ian, others. >>> >>> Hi Ilya, >>> >>>> Following up from a brief conversation during today's upstream meeting. >>>> It was brought to my attention that you're expecting this series and >>>> the 'hash' one to be accepted into 2.17. Though there are few issues >>>> with that: >>>> >>>> 1. This review for v4 was actually very first review of the patch set. >>>> The other one as of today doesn't have any reviews at all. >>>> Looking at the change log for this patch set it doesn't seem that >>>> internal reviews behind the closed doors (if there were any) requested >>>> any significant changes. In any case, internal reviews is not the way >>>> how open-source projects work. >>> >>> Actions & MFEX were developed internally yes, and hence internal reviews and >>> architecture was iterated on. Saying "not many large changes requested" is not >> relevant, >>> it means that internally the architecture was well aligned. If anything, it means >> that >>> reviewers did not have big concerns, hence we should have better confidence >> to merge. >> >> Me, who never seen these reviews and architecture discussions has exactly >> zero confidence in these patch-set the same as the rest of the community. >> Development behind closed doors is never taken into account while making >> decisions in open-source projects, because this development and made >> decisions are not visible and can not be peer-reviewed by anyone outside. >> >> On a quick glance over the code and reviews done in a last couple of days, >> I'd also question the quality of these public reviews as the patch set >> at least contains a few issues in a copy-pasted code that was already >> fixed on master branch (left alone that these code duplications should, >> likley, not exist in a first place). >> >>> >>> >>>> 2. The soft freeze for 2.17 began on Jan 3 in accordance with our >>>> release schedule (even a bit later), and as you know, during the soft >>>> freeze we're not normally accepting patches that wasn't already reviewed >>>> before the soft freeze begun. >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390487.html >>> >>> Actions and MFEX Hashing were discussed and reviewed in public at OVS Conf; >>> https://www.openvswitch.org/support/ovscon2021/#T32 >>> https://www.openvswitch.org/support/ovscon2021/#T33 >> >> They were presented, but not discussed or reviewed. >> >>> >>> The closing "call to action" slide clearly states 2.17 upstream is intended, >>> and welcome community review & comments, no concerns were raised. >>> >>>> That's not the end of a world, but you need to request an exception in >>>> reply to the email linked above. >>> >>> As both the patches and intent to merge for OVS 2.17 were clearly discussed >>> in public at OVS Conf, this "request exception" case does not apply. >> >> Again, they were presented, not discussed. It's the same as sending patches >> to the mailing list. We don't have a policy to accept everything that >> wasn't explicitly rejected. Lack of feedback / responses in most cases means >> lack of interest. >> >>>> But I have a few high-level concerns regarding the patch set itself, >>>> and that's a bigger problem for me: >>>> >>>> 1. What are the benefits of these patch sets? A lot of infrastructure >>>> changes are made, but the benefits of them are unclear. Why these >>>> changes are needed in the end? I believe, that was the main reason >>>> why community had no interest in reviewing these patches. >>>> 2.17 is supposed to be a new LTS, so infrastructure changes without >>>> clear benefits might not be a good fit taking into account time >>>> constraints and lack of reviews. >>> >>> Customers workloads are accelerated, improving OVS datapath performance >> for their >>> workloads. The customers are not active in SW development themselves, >> hence there >>> is no reviews from them on OVS ML. >> >> I don't see anything being accelerated. I'm not asking your customers to >> prove anything, I'm asking you and your team to provide a reason to review >> the patches. "Workloads are accelerated" is a blunt statement without >> any numbers. And even that you're saying for a very first time. For what I >> know it could be 0.00001% improvement that simply doesn't worth it. I don't >> see any claims about performance regression testing being made either. >> >>> >>> If you have concerns over a patchset, these should be raised against a V1 of a >> patchset, >> >> I don't have time to look at every patch-set that doesn't even try to make >> me or anyone else interested in reviewing it and you're re-spinning the >> series without any reviews being made, so that is simply not practical. >> >>> in appropriate time to be addressed. Asking these questions the week of the >> integration >>> deadline is unacceptable, and may not be used to block merge a patchset. >> >> Again, there is no policy to accept everything that wasn't explicitly rejected. >> You missed the review deadline for a soft freeze. So, these patches was >> not considered for integration at all at this point. Hence questioning them >> is justified. >> >>> >>> >>>> 2. The same concern that I already brought to you attention in other >>>> conversations, e.g. on the ovs-security list about a month ago. >>>> It's related to all developments in that area: why this is tied to >>>> the userspace datapath? i.e. why execution of actions depends on the >>>> datapath? This seems artificial and complicates testing a lot. >>>> Like current autovalidator is not able to test most of the packet >>>> parsing cases, the same way autovalidator will not be able to test >>>> execution of actions. >>> >>> Regarding testing, as per that same conversation, I replied that yes we would >> investigate >>> improving testing even further. There is ongoing work here around improving >> our fuzz testing. >> >> Didn't see any progress from your side on that front. And still waiting for >> your reply on this email: >> https://mail.openvswitch.org/pipermail/ovs-dev/2021-December/389945.html >> >>> >>> Autovalidation is not complex, it validates all unit-tests, and has been >> presented at OVS >> >> 'all unit tests' is a plain lie here. You're perfectly aware that this is >> not true at least for miniflow_extract part. > > Please avoid confronting language such as "is a plain lie" and suggesting that > I'm purposely trying to mislead, that is not my intent. > > I expect all OVS community members to be respectful, and especially the OVS Maintainers. > The above comments have no place in an open-source community such as OVS. > > If there is a misunderstanding around the above statement, ask for more explanation, and we > can discuss it in a professional and polite manner. On my side, I would say that phrases like 'is unacceptable' are a confronting language and I'm simply responding to them accordingly. I'm not normally using such words, and my initial reply on Jan 12, IMO, was polite and professional, and even suggested a way out from a soft freeze situation. I don't know why you're trying to turn every conversation into a battle. If instead of demands, requests and accusations we will ask and negotiate, everyone will be much happier. > > >>> conferences, and upstreamed for multiple components (DPCLS, MFEX, and >> now Actions). >>> If there is concern over approach, then this should not be held against Actions >> patchset >>> now in the week of merge, but addressed in general. >> >> AFAICT, you're not planning to address concerns in general, because the issue >> with miniflow_extract autovalidation not being usable for a big chunk of existing >> tests was flagged several times including the email mentioned above, and you're >> always avoiding to reply to these concerns. >> >>> Not acceptable to block Actions >>> or Hashing patchsets based on this, in the week of the integration deadline. >> >> Continuing to re-use and build new things on top of broken abstractions >> doesn't look good to me. >> >>> >>> >>>> I have some more comments, but they are more related to the actual code >>>> and above 2 are the most important for now. >>>> >>>> Best regards, Ilya Maximets. >>> >>> Regards, -Harry > > > This patchset has received significant push-back, both on this thread and in > Flavio's reply here; https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390732.html > > I feel that there needs to be a clearer path to success in getting contributions merged upstream in OVS. > In particular, concerns need to be raised earlier and not in the last week before merge deadline. Everything is simple. Participate in the community, review patches, so others will have time looking at yours. And make your patches interesting to review; performance optimizations without any numbers doesn't make a lot of sense. No-one should be obliged to do what is not interesting to them, and most of the work done by the community (including me) is voluntary, especially reviews. And we still have a gap between the rate of posted and reviewed patches, so some of them will inevitably be left without attention. And you, the member of community, is the only person who can fix that by contributing your time. > > On closing this thread for 2.17, please note we will continue to upstream these Actions optimizations for > OVS 2.18, and request constructive and actionable feedback in order to achieve any rework required for merging. In this context, I'm also considering 'we request' as a confronting language. And that immediately turns me away from trying to help. So, if being respectful is your standard, try to live up to it. On the subject, most of the top level comments was already made, so you're free to start working on them. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday, January 14, 2022 8:17 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > <kumar.amber@intel.com> > Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com>; Mcnamara, John > <john.mcnamara@intel.com> > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/14/22 18:11, Van Haaren, Harry wrote: > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Thursday, January 13, 2022 1:14 PM > >> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > >> <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > >> <kumar.amber@intel.com> > >> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > >> <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com>; Mcnamara, John > >> <john.mcnamara@intel.com> > >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> On 1/13/22 11:53, Van Haaren, Harry wrote: > >>>> -----Original Message----- > >>>> From: Ilya Maximets <i.maximets@ovn.org> > >>>> Sent: Wednesday, January 12, 2022 6:01 PM > >>>> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Finn, Emma > >>>> <emma.finn@intel.com>; dev@openvswitch.org; Amber, Kumar > >>>> <kumar.amber@intel.com> > >>>> Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Flavio Leitner > >>>> <fbl@sysclose.org>; Kevin Traynor <ktraynor@redhat.com> > >>>> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >>>> > >>>> On 1/6/22 14:11, Van Haaren, Harry wrote: > >>>>>> -----Original Message----- > >>>>>> From: Finn, Emma <emma.finn@intel.com> > >>>>>> Sent: Wednesday, January 5, 2022 4:54 PM > >>>>>> To: dev@openvswitch.org; Van Haaren, Harry > >> <harry.van.haaren@intel.com>; > >>>>>> Amber, Kumar <kumar.amber@intel.com> > >>>>>> Cc: Finn, Emma <emma.finn@intel.com> > >>>>>> Subject: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >>>>>> > >>>>>> --- > >>>>>> v4: > >>>>>> - Rebase to master > >>>>>> - Add ISA implementation of push_vlan action > >>>>> > >>>>> Thanks for the updated patchset Emma & Amber. > >>>>> > >>>>> Overall, this is working as expected and I've only had some minor > >>>>> comments throughout the patchset. I've added my Acked-by to most > >>>>> patches, some small open questions remain to be addressed in a v5. > >>>>> > >>>>> +CC Ian/Ilya , I'd like to see the v5 get merged, so let's continue to work > >>>> towards that. > >>>> > >>>> Hi, Harry, Ian, others. > >>> > >>> Hi Ilya, > >>> > >>>> Following up from a brief conversation during today's upstream meeting. > >>>> It was brought to my attention that you're expecting this series and > >>>> the 'hash' one to be accepted into 2.17. Though there are few issues > >>>> with that: > >>>> > >>>> 1. This review for v4 was actually very first review of the patch set. > >>>> The other one as of today doesn't have any reviews at all. > >>>> Looking at the change log for this patch set it doesn't seem that > >>>> internal reviews behind the closed doors (if there were any) requested > >>>> any significant changes. In any case, internal reviews is not the way > >>>> how open-source projects work. > >>> > >>> Actions & MFEX were developed internally yes, and hence internal reviews > and > >>> architecture was iterated on. Saying "not many large changes requested" is > not > >> relevant, > >>> it means that internally the architecture was well aligned. If anything, it > means > >> that > >>> reviewers did not have big concerns, hence we should have better > confidence > >> to merge. > >> > >> Me, who never seen these reviews and architecture discussions has exactly > >> zero confidence in these patch-set the same as the rest of the community. > >> Development behind closed doors is never taken into account while making > >> decisions in open-source projects, because this development and made > >> decisions are not visible and can not be peer-reviewed by anyone outside. > >> > >> On a quick glance over the code and reviews done in a last couple of days, > >> I'd also question the quality of these public reviews as the patch set > >> at least contains a few issues in a copy-pasted code that was already > >> fixed on master branch (left alone that these code duplications should, > >> likley, not exist in a first place). > >> > >>> > >>> > >>>> 2. The soft freeze for 2.17 began on Jan 3 in accordance with our > >>>> release schedule (even a bit later), and as you know, during the soft > >>>> freeze we're not normally accepting patches that wasn't already reviewed > >>>> before the soft freeze begun. > >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2022- > January/390487.html > >>> > >>> Actions and MFEX Hashing were discussed and reviewed in public at OVS > Conf; > >>> https://www.openvswitch.org/support/ovscon2021/#T32 > >>> https://www.openvswitch.org/support/ovscon2021/#T33 > >> > >> They were presented, but not discussed or reviewed. > >> > >>> > >>> The closing "call to action" slide clearly states 2.17 upstream is intended, > >>> and welcome community review & comments, no concerns were raised. > >>> > >>>> That's not the end of a world, but you need to request an exception in > >>>> reply to the email linked above. > >>> > >>> As both the patches and intent to merge for OVS 2.17 were clearly discussed > >>> in public at OVS Conf, this "request exception" case does not apply. > >> > >> Again, they were presented, not discussed. It's the same as sending patches > >> to the mailing list. We don't have a policy to accept everything that > >> wasn't explicitly rejected. Lack of feedback / responses in most cases means > >> lack of interest. > >> > >>>> But I have a few high-level concerns regarding the patch set itself, > >>>> and that's a bigger problem for me: > >>>> > >>>> 1. What are the benefits of these patch sets? A lot of infrastructure > >>>> changes are made, but the benefits of them are unclear. Why these > >>>> changes are needed in the end? I believe, that was the main reason > >>>> why community had no interest in reviewing these patches. > >>>> 2.17 is supposed to be a new LTS, so infrastructure changes without > >>>> clear benefits might not be a good fit taking into account time > >>>> constraints and lack of reviews. > >>> > >>> Customers workloads are accelerated, improving OVS datapath performance > >> for their > >>> workloads. The customers are not active in SW development themselves, > >> hence there > >>> is no reviews from them on OVS ML. > >> > >> I don't see anything being accelerated. I'm not asking your customers to > >> prove anything, I'm asking you and your team to provide a reason to review > >> the patches. "Workloads are accelerated" is a blunt statement without > >> any numbers. And even that you're saying for a very first time. For what I > >> know it could be 0.00001% improvement that simply doesn't worth it. I don't > >> see any claims about performance regression testing being made either. > >> > >>> > >>> If you have concerns over a patchset, these should be raised against a V1 of a > >> patchset, > >> > >> I don't have time to look at every patch-set that doesn't even try to make > >> me or anyone else interested in reviewing it and you're re-spinning the > >> series without any reviews being made, so that is simply not practical. > >> > >>> in appropriate time to be addressed. Asking these questions the week of the > >> integration > >>> deadline is unacceptable, and may not be used to block merge a patchset. > >> > >> Again, there is no policy to accept everything that wasn't explicitly rejected. > >> You missed the review deadline for a soft freeze. So, these patches was > >> not considered for integration at all at this point. Hence questioning them > >> is justified. > >> > >>> > >>> > >>>> 2. The same concern that I already brought to you attention in other > >>>> conversations, e.g. on the ovs-security list about a month ago. > >>>> It's related to all developments in that area: why this is tied to > >>>> the userspace datapath? i.e. why execution of actions depends on the > >>>> datapath? This seems artificial and complicates testing a lot. > >>>> Like current autovalidator is not able to test most of the packet > >>>> parsing cases, the same way autovalidator will not be able to test > >>>> execution of actions. > >>> > >>> Regarding testing, as per that same conversation, I replied that yes we would > >> investigate > >>> improving testing even further. There is ongoing work here around > improving > >> our fuzz testing. > >> > >> Didn't see any progress from your side on that front. And still waiting for > >> your reply on this email: > >> https://mail.openvswitch.org/pipermail/ovs-dev/2021- > December/389945.html > >> > >>> > >>> Autovalidation is not complex, it validates all unit-tests, and has been > >> presented at OVS > >> > >> 'all unit tests' is a plain lie here. You're perfectly aware that this is > >> not true at least for miniflow_extract part. > > > > Please avoid confronting language such as "is a plain lie" and suggesting that > > I'm purposely trying to mislead, that is not my intent. > > > > I expect all OVS community members to be respectful, and especially the OVS > Maintainers. > > The above comments have no place in an open-source community such as OVS. > > > > If there is a misunderstanding around the above statement, ask for more > explanation, and we > > can discuss it in a professional and polite manner. > > On my side, I would say that phrases like 'is unacceptable' are a > confronting language and I'm simply responding to them accordingly. > I'm not normally using such words, and my initial reply on Jan 12, > IMO, was polite and professional, and even suggested a way out from > a soft freeze situation. I don't know why you're trying to turn > every conversation into a battle. > > If instead of demands, requests and accusations we will ask and > negotiate, everyone will be much happier. > > > > > > >>> conferences, and upstreamed for multiple components (DPCLS, MFEX, and > >> now Actions). > >>> If there is concern over approach, then this should not be held against > Actions > >> patchset > >>> now in the week of merge, but addressed in general. > >> > >> AFAICT, you're not planning to address concerns in general, because the issue > >> with miniflow_extract autovalidation not being usable for a big chunk of > existing > >> tests was flagged several times including the email mentioned above, and > you're > >> always avoiding to reply to these concerns. > >> > >>> Not acceptable to block Actions > >>> or Hashing patchsets based on this, in the week of the integration deadline. > >> > >> Continuing to re-use and build new things on top of broken abstractions > >> doesn't look good to me. > >> > >>> > >>> > >>>> I have some more comments, but they are more related to the actual code > >>>> and above 2 are the most important for now. > >>>> > >>>> Best regards, Ilya Maximets. > >>> > >>> Regards, -Harry > > > > > > This patchset has received significant push-back, both on this thread and in > > Flavio's reply here; https://mail.openvswitch.org/pipermail/ovs-dev/2022- > January/390732.html > > > > I feel that there needs to be a clearer path to success in getting contributions > merged upstream in OVS. > > In particular, concerns need to be raised earlier and not in the last week before > merge deadline. > > Everything is simple. Participate in the community, review patches, > so others will have time looking at yours. And make your patches > interesting to review; performance optimizations without any numbers > doesn't make a lot of sense. No-one should be obliged to do what is > not interesting to them, and most of the work done by the community > (including me) is voluntary, especially reviews. > > And we still have a gap between the rate of posted and reviewed patches, > so some of them will inevitably be left without attention. And you, > the member of community, is the only person who can fix that by > contributing your time. > > > > > On closing this thread for 2.17, please note we will continue to upstream these > Actions optimizations for > > OVS 2.18, and request constructive and actionable feedback in order to achieve > any rework required for merging. > > In this context, I'm also considering 'we request' as a confronting > language. And that immediately turns me away from trying to help. > So, if being respectful is your standard, try to live up to it. > > On the subject, most of the top level comments was already made, so > you're free to start working on them. > > Best regards, Ilya Maximets. This email is to indicate that I have read the reply and although I don't agree with some of the statements above, it seems time to move on and discuss technical concerns to allows merging code upstream into OVS.
--- v4: - Rebase to master - Add ISA implementation of push_vlan action --- v3: - Refactored to fix unit test failures - Removed some sign-off on commits --- v2: - Fix the CI build issues --- This patchset introduces actions infrastructure changes which allows the user to choose between different action implementations based on CPU ISA by using different commands. The Infrastructure also provides a way to check the correctness of the ISA optimized action version against the scalar version. This patchset also introduces an optimized version of the push and pop vlan actions. Emma Finn (7): odp-execute: Add function pointers to odp-execute for different action implementations. odp-execute: Add function pointer for pop_vlan action. odp-execute: Add auto validation function for actions. odp-execute: Add command to switch action implementation. odp-execute: Add ISA implementation of actions. odp-execute: Add ISA implementation of pop_vlan action. odp-execute: Add ISA implementation of push_vlan action. Kumar Amber (2): pmd.at: Add test-cases for ovs-actions commands. dpif-netdev: Add configure option to enable actions autovalidator at build time. Documentation/topics/dpdk/bridge.rst | 25 +++ Documentation/topics/testing.rst | 20 ++- NEWS | 9 ++ acinclude.m4 | 17 ++ configure.ac | 1 + lib/automake.mk | 6 +- lib/cpu.c | 1 + lib/cpu.h | 1 + lib/dp-packet.c | 23 +++ lib/dp-packet.h | 5 + lib/dpif-netdev-unixctl.man | 6 + lib/dpif-netdev.c | 41 +++++ lib/odp-execute-avx512.c | 202 ++++++++++++++++++++++++ lib/odp-execute-private.c | 224 +++++++++++++++++++++++++++ lib/odp-execute-private.h | 110 +++++++++++++ lib/odp-execute.c | 108 ++++++++++--- lib/odp-execute.h | 9 ++ tests/pmd.at | 20 +++ 18 files changed, 800 insertions(+), 28 deletions(-) create mode 100644 lib/odp-execute-avx512.c create mode 100644 lib/odp-execute-private.c create mode 100644 lib/odp-execute-private.h