On 7/12/22 16:09, Stokes, Ian wrote:
> Hi Folks,
>
> Any response to these queries? I’m uncomfortable TBH with this feature going
> in to 2.18 unless these are addressed/resolved. From an Intel perspective
> we’re more than happy to test and review but there are genuine issues to be
> discussed I feel below before merge.
I tend to agree here. The patch set doesn't seem to be ready.
It has a few major issues like broken upgrades/live migration
and doesn't seem to be tested enough to enable features by
default.
I'd suggest to not rush it, but take a closer look at what is
going on, write good unit tests for the "untested" software
fallback, which is a major concern. Since lots of refactoring
and changes are happening at the same time, it's better to
give it time to soak before enabling by default. It's fine
to enable if it gives better performance for everyone, but
we clearly don't have enough time to assess that.
I'm generally also not very comfortable with changing historical
names like 'p' and 'b' in functions just for the sake of renaming.
And I agree with Amber that some cover letter would be nice to
have. We should also have performance test results somewhere
to be sure that we're not introducing regressions. Performance
tests for the software fallback are also necessary.
To be clear, I didn't really review the patch set, but a quick
glance over patches and discussions doesn't give me any
confidence at this point.
Best regards, Ilya Maximets.
P.S. The original email from Amber might have got lost since it
wasn't a reply, but a standalone email instead.
>
> Thanks
> Ian
>
> *From:* Amber, Kumar <[email protected]>
> *Sent:* Thursday, July 7, 2022 10:52 AM
> *To:* [email protected]; [email protected]; [email protected]
> *Cc:* Ilya Maximets <[email protected]>; Van Haaren, Harry
> <[email protected]>; Stokes, Ian <[email protected]>
> *Subject:* [ovs-dev] [PATCH 1/14] Rename flags with CKSUM to CSUM
>
> Hi Flavio, Mike,
>
> I did have a quick look over the patch-set
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=307485
> <https://patchwork.ozlabs.org/project/openvswitch/list/?series=307485>
>
> I have some comments over the patches mentioned below:
>
> 1. Can a Cover letter be provided with the patch-set to
> understand the intent/approach of patch-set to the problem?
> a. A cover letter would give the series a better
> title, and a place to give a high-level overview of approach.
> 2. There are two big concerns with the patch-set:
> a. The SW fallback patch is marked as an
> _untested_ patch to showcase the proposed solution. Given its untested
> status, we assume it is not a candidate for 2.18.
> b. Changing of default behavior of TSO/GSO to
> default ON, why is this necessary? Features are typically "opt in", what
> makes TSO/GSO different?
> 3. Can the patch-set be split into 2 logical parts being CSUM
> and TSO/GSO, any thoughts on this?
>
> Regards
> Amber
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
