Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
> -Original Message- > From: Ilya Maximets > Sent: Friday, January 14, 2022 8:17 PM > To: Van Haaren, Harry ; Finn, Emma > ; d...@openvswitch.org; Amber, Kumar > > Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner > ; Kevin Traynor ; Mcnamara, John > > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/14/22 18:11, Van Haaren, Harry wrote: > >> -Original Message- > >> From: Ilya Maximets > >> Sent: Thursday, January 13, 2022 1:14 PM > >> To: Van Haaren, Harry ; Finn, Emma > >> ; d...@openvswitch.org; Amber, Kumar > >> > >> Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner > >> ; Kevin Traynor ; Mcnamara, John > >> > >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> On 1/13/22 11:53, Van Haaren, Harry wrote: > -Original Message- > From: Ilya Maximets > Sent: Wednesday, January 12, 2022 6:01 PM > To: Van Haaren, Harry ; Finn, Emma > ; d...@openvswitch.org; Amber, Kumar > > Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio > Leitner > ; Kevin Traynor > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/6/22 14:11, Van Haaren, Harry wrote: > >> -Original Message- > >> From: Finn, Emma > >> Sent: Wednesday, January 5, 2022 4:54 PM > >> To: d...@openvswitch.org; Van Haaren, Harry > >> ; > >> Amber, Kumar > >> Cc: Finn, Emma > >> 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 >
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
On 1/14/22 18:11, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Thursday, January 13, 2022 1:14 PM >> To: Van Haaren, Harry ; Finn, Emma >> ; d...@openvswitch.org; Amber, Kumar >> >> Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner >> ; Kevin Traynor ; Mcnamara, John >> >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> On 1/13/22 11:53, Van Haaren, Harry wrote: -Original Message- From: Ilya Maximets Sent: Wednesday, January 12, 2022 6:01 PM To: Van Haaren, Harry ; Finn, Emma ; d...@openvswitch.org; Amber, Kumar Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner ; Kevin Traynor Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations On 1/6/22 14:11, Van Haaren, Harry wrote: >> -Original Message- >> From: Finn, Emma >> Sent: Wednesday, January 5, 2022 4:54 PM >> To: d...@openvswitch.org; Van Haaren, Harry >> ; >> Amber, Kumar >> Cc: Finn, Emma >> 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
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
> -Original Message- > From: Ilya Maximets > Sent: Thursday, January 13, 2022 1:14 PM > To: Van Haaren, Harry ; Finn, Emma > ; d...@openvswitch.org; Amber, Kumar > > Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner > ; Kevin Traynor ; Mcnamara, John > > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/13/22 11:53, Van Haaren, Harry wrote: > >> -Original Message- > >> From: Ilya Maximets > >> Sent: Wednesday, January 12, 2022 6:01 PM > >> To: Van Haaren, Harry ; Finn, Emma > >> ; d...@openvswitch.org; Amber, Kumar > >> > >> Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner > >> ; Kevin Traynor > >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > >> > >> On 1/6/22 14:11, Van Haaren, Harry wrote: > -Original Message- > From: Finn, Emma > Sent: Wednesday, January 5, 2022 4:54 PM > To: d...@openvswitch.org; Van Haaren, Harry > ; > Amber, Kumar > Cc: Finn, Emma > 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
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
On 1/13/22 11:53, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Wednesday, January 12, 2022 6:01 PM >> To: Van Haaren, Harry ; Finn, Emma >> ; d...@openvswitch.org; Amber, Kumar >> >> Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner >> ; Kevin Traynor >> Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations >> >> On 1/6/22 14:11, Van Haaren, Harry wrote: -Original Message- From: Finn, Emma Sent: Wednesday, January 5, 2022 4:54 PM To: d...@openvswitch.org; Van Haaren, Harry ; Amber, Kumar Cc: Finn, Emma 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
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
Hi, On Thu, Jan 13, 2022 at 10:53:57AM +, Van Haaren, Harry wrote: > > -Original Message- > > From: Ilya Maximets > > Sent: Wednesday, January 12, 2022 6:01 PM > > To: Van Haaren, Harry ; Finn, Emma > > ; d...@openvswitch.org; Amber, Kumar > > > > Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner > > ; Kevin Traynor > > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > > > On 1/6/22 14:11, Van Haaren, Harry wrote: > > >> -Original Message- > > >> From: Finn, Emma > > >> Sent: Wednesday, January 5, 2022 4:54 PM > > >> To: d...@openvswitch.org; Van Haaren, Harry ; > > >> Amber, Kumar > > >> Cc: Finn, Emma > > >> 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
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
> -Original Message- > From: Ilya Maximets > Sent: Wednesday, January 12, 2022 6:01 PM > To: Van Haaren, Harry ; Finn, Emma > ; d...@openvswitch.org; Amber, Kumar > > Cc: i.maxim...@ovn.org; Stokes, Ian ; Flavio Leitner > ; Kevin Traynor > Subject: Re: [PATCH v4 0/9] Actions Infrastructure + Optimizations > > On 1/6/22 14:11, Van Haaren, Harry wrote: > >> -Original Message- > >> From: Finn, Emma > >> Sent: Wednesday, January 5, 2022 4:54 PM > >> To: d...@openvswitch.org; Van Haaren, Harry ; > >> Amber, Kumar > >> Cc: Finn, Emma > >> 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
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
On 1/6/22 14:11, Van Haaren, Harry wrote: >> -Original Message- >> From: Finn, Emma >> Sent: Wednesday, January 5, 2022 4:54 PM >> To: d...@openvswitch.org; Van Haaren, Harry ; >> Amber, Kumar >> Cc: Finn, Emma >> 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
> -Original Message- > From: Finn, Emma > Sent: Wednesday, January 5, 2022 4:54 PM > To: d...@openvswitch.org; Van Haaren, Harry ; > Amber, Kumar > Cc: Finn, Emma > 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 0/9] Actions Infrastructure + Optimizations
--- 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 -- 2.25.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
