[ovs-dev,v4] datapath-windows: Enable checksum offloads in STT
diff mbox

Message ID 1442867534-10168-1-git-send-email-vsairam@vmware.com
State Accepted
Headers show

Commit Message

Sairam Venugopal Sept. 21, 2015, 8:32 p.m. UTC
Enable support for Checksum offloads in STT if it's enabled in the Windows
VM. Set the Checksum Partial and Checksum Verified flags as mentioned in
the STT Draft - https://tools.ietf.org/html/draft-davie-stt-06

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
---
 datapath-windows/ovsext/Stt.c | 173 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 165 insertions(+), 8 deletions(-)

Comments

Nithin Raju Sept. 21, 2015, 10:02 p.m. UTC | #1
> On Sep 21, 2015, at 1:32 PM, Sairam Venugopal <vsairam@vmware.com> wrote:
> 
> Enable support for Checksum offloads in STT if it's enabled in the Windows
> VM. Set the Checksum Partial and Checksum Verified flags as mentioned in
> the STT Draft - https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Ddavie-2Dstt-2D06&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=ico0M9o__zbW4WsRoCYDeJPyrIqViqAfc4tuFgAb2no&s=NBtZP8VMRSX7rWjo0KQ-gvG2z5NMBp0761SssqzE1vA&e= 
> 
> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>

Thanks for addressing the comments.

Acked-by: Nithin Raju <nithin@vmware.com>
Sairam Venugopal Sept. 21, 2015, 10:06 p.m. UTC | #2
Hey Nithin,

Thanks for reviewing the patch.

Regards,
Sairam

On 9/21/15, 3:02 PM, "Nithin Raju" <nithin@vmware.com> wrote:

>> On Sep 21, 2015, at 1:32 PM, Sairam Venugopal <vsairam@vmware.com>
>>wrote:
>> 
>> Enable support for Checksum offloads in STT if it's enabled in the
>>Windows
>> VM. Set the Checksum Partial and Checksum Verified flags as mentioned in
>> the STT Draft - 
>>https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_
>>draft-2Ddavie-2Dstt-2D06&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNt
>>Xt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=ico0M9o__zbW4WsRoC
>>YDeJPyrIqViqAfc4tuFgAb2no&s=NBtZP8VMRSX7rWjo0KQ-gvG2z5NMBp0761SssqzE1vA&e
>>= 
>> 
>> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
>
>Thanks for addressing the comments.
>
>Acked-by: Nithin Raju <nithin@vmware.com>
Ben Pfaff Sept. 22, 2015, 4:07 p.m. UTC | #3
Thanks Sairam and Nithin, I applied this to master.

On Mon, Sep 21, 2015 at 10:06:33PM +0000, Sairam Venugopal wrote:
> Hey Nithin,
> 
> Thanks for reviewing the patch.
> 
> Regards,
> Sairam
> 
> On 9/21/15, 3:02 PM, "Nithin Raju" <nithin@vmware.com> wrote:
> 
> >> On Sep 21, 2015, at 1:32 PM, Sairam Venugopal <vsairam@vmware.com>
> >>wrote:
> >> 
> >> Enable support for Checksum offloads in STT if it's enabled in the
> >>Windows
> >> VM. Set the Checksum Partial and Checksum Verified flags as mentioned in
> >> the STT Draft - 
> >>https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_
> >>draft-2Ddavie-2Dstt-2D06&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNt
> >>Xt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=ico0M9o__zbW4WsRoC
> >>YDeJPyrIqViqAfc4tuFgAb2no&s=NBtZP8VMRSX7rWjo0KQ-gvG2z5NMBp0761SssqzE1vA&e
> >>= 
> >> 
> >> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
> >
> >Thanks for addressing the comments.
> >
> >Acked-by: Nithin Raju <nithin@vmware.com>
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Nithin Raju Sept. 22, 2015, 4:18 p.m. UTC | #4
> On Sep 22, 2015, at 9:07 AM, Ben Pfaff <blp@nicira.com> wrote:
> 
> Thanks Sairam and Nithin, I applied this to master.

Ben,
Can you pls. apply this to 2.4 as well?

thanks,
-- Nithin
Ben Pfaff Sept. 22, 2015, 4:36 p.m. UTC | #5
On Tue, Sep 22, 2015 at 04:18:03PM +0000, Nithin Raju wrote:
> 
> > On Sep 22, 2015, at 9:07 AM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > Thanks Sairam and Nithin, I applied this to master.
> 
> Ben,
> Can you pls. apply this to 2.4 as well?

Done!

Thanks,

Ben.
Jesse Gross Sept. 23, 2015, 2:11 a.m. UTC | #6
On Tue, Sep 22, 2015 at 9:18 AM, Nithin Raju <nithin@vmware.com> wrote:
>
>> On Sep 22, 2015, at 9:07 AM, Ben Pfaff <blp@nicira.com> wrote:
>>
>> Thanks Sairam and Nithin, I applied this to master.
>
> Ben,
> Can you pls. apply this to 2.4 as well?

I'm somewhat concerned about the number and size of Windows patches
that are targeted at 2.4 (it seems a lot went out today in
particular). Besides being large, many of them don't seem to meet the
criteria that I would normally expect for a backport. For example,
some look like features (64 bit support, TCP flags) while others
appear to be fixes for bugs so fundamental to the operation of things
that it seems unlikely that that part of the code can be deployed as
it currently exists (checksums in STT, VXLAN).

Can you please explain the rationale?
Nithin Raju Sept. 23, 2015, 6:04 a.m. UTC | #7
hi Jesse,
We are getting the Hyper-V solution to a state with the following goals:
- Work “out of the box” ie. no need to make special settings such as disabling checksum offload, TSO, etc.
- Reasonably stable

Most of the patches we have checked in so far into 2.4 are geared towards these two goals. Once all of the required changes go in, and we are reasonably confident about the stability, we can hopefully make an announcement about Hyper-V support.

The 64-bit support is geared towards support Windows Nano which IIRC, supports only 64-bit apps (Alin, correct me if I’m wrong).

STT and VXLAN checksumming patches are required to make sure that we don’t have to disable them at the VIF to make TCP/Ping traffic work.

The TCP flags patch is probably optional for 2.4, but maybe Alin has a good reason for it.

I agree that we should not destabilize 2.4 branch and we’ll take precautions for it.

Pls. let us know if you have concerns.

thanks,
-- Nithin

> On Sep 22, 2015, at 7:11 PM, Jesse Gross <jesse@nicira.com> wrote:

> 

> On Tue, Sep 22, 2015 at 9:18 AM, Nithin Raju <nithin@vmware.com> wrote:

>> 

>>> On Sep 22, 2015, at 9:07 AM, Ben Pfaff <blp@nicira.com> wrote:

>>> 

>>> Thanks Sairam and Nithin, I applied this to master.

>> 

>> Ben,

>> Can you pls. apply this to 2.4 as well?

> 

> I'm somewhat concerned about the number and size of Windows patches

> that are targeted at 2.4 (it seems a lot went out today in

> particular). Besides being large, many of them don't seem to meet the

> criteria that I would normally expect for a backport. For example,

> some look like features (64 bit support, TCP flags) while others

> appear to be fixes for bugs so fundamental to the operation of things

> that it seems unlikely that that part of the code can be deployed as

> it currently exists (checksums in STT, VXLAN).

> 

> Can you please explain the rationale?
Alin Serdean Sept. 23, 2015, 1:33 p.m. UTC | #8
I added my comments inlined.

> -----Mesaj original-----

> De la: Nithin Raju [mailto:nithin@vmware.com]

> Trimis: Wednesday, September 23, 2015 9:05 AM

> Către: Jesse Gross <jesse@nicira.com>

> Cc: Alin Serdean <aserdean@cloudbasesolutions.com>; Ben Pfaff

> <blp@nicira.com>; dev@openvswitch.org

> Subiect: Re: [ovs-dev] [PATCH v4] datapath-windows: Enable checksum

> offloads in STT

> 

> hi Jesse,

> We are getting the Hyper-V solution to a state with the following goals:

> - Work “out of the box” ie. no need to make special settings such as disabling

> checksum offload, TSO, etc.

> - Reasonably stable

> 

> Most of the patches we have checked in so far into 2.4 are geared towards

> these two goals. Once all of the required changes go in, and we are

> reasonably confident about the stability, we can hopefully make an

> announcement about Hyper-V support.

[Alin Gabriel Serdean: ] +1
> 

> The 64-bit support is geared towards support Windows Nano which IIRC,

> supports only 64-bit apps (Alin, correct me if I’m wrong).

[Alin Gabriel Serdean: ] 64-bit is targeted mostly for NanoServer and to some
extent it increases performance.
It is important that we have a stable branch that supports NanoServers
especially for testing purposes because of the low disk requirements it offers.
(http://blogs.technet.com/b/windowsserver/archive/2015/04/08/microsoft-announces-nano-server-for-modern-apps-and-cloud.aspx)
> 

> STT and VXLAN checksumming patches are required to make sure that we

> don’t have to disable them at the VIF to make TCP/Ping traffic work.

[Alin Gabriel Serdean: ] I concur this is targeted directly for the users and it
will ease up a lot of configuration headaches when deploying.
> 

> The TCP flags patch is probably optional for 2.4, but maybe Alin has a good

> reason for it.

[Alin Gabriel Serdean: ] This is a full flat feature. I just thought it would be nice
to have it on both branches.
> 

> I agree that we should not destabilize 2.4 branch and we’ll take precautions

> for it.

[Alin Gabriel Serdean: ] +1
> 

> Pls. let us know if you have concerns.

> 

> thanks,

> -- Nithin

> 

> > On Sep 22, 2015, at 7:11 PM, Jesse Gross <jesse@nicira.com> wrote:

> >

> > On Tue, Sep 22, 2015 at 9:18 AM, Nithin Raju <nithin@vmware.com> wrote:

> >>

> >>> On Sep 22, 2015, at 9:07 AM, Ben Pfaff <blp@nicira.com> wrote:

> >>>

> >>> Thanks Sairam and Nithin, I applied this to master.

> >>

> >> Ben,

> >> Can you pls. apply this to 2.4 as well?

> >

> > I'm somewhat concerned about the number and size of Windows patches

> > that are targeted at 2.4 (it seems a lot went out today in

> > particular). Besides being large, many of them don't seem to meet the

> > criteria that I would normally expect for a backport. For example,

> > some look like features (64 bit support, TCP flags) while others

> > appear to be fixes for bugs so fundamental to the operation of things

> > that it seems unlikely that that part of the code can be deployed as

> > it currently exists (checksums in STT, VXLAN).

> >

> > Can you please explain the rationale?
Jesse Gross Sept. 23, 2015, 3:33 p.m. UTC | #9
On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com> wrote:
> hi Jesse,
> We are getting the Hyper-V solution to a state with the following goals:
> - Work “out of the box” ie. no need to make special settings such as disabling checksum offload, TSO, etc.
> - Reasonably stable
>
> Most of the patches we have checked in so far into 2.4 are geared towards these two goals. Once all of the required changes go in, and we are reasonably confident about the stability, we can hopefully make an announcement about Hyper-V support.

This is my concern - there should not be any announcements based on
stable branches because there should be no development on stable
branches. The only thing that should go in is targeted bug fixes to
address issues that came up after the release.

All of the goals that you listed are good things and make sense - on
the master branch. However, I don't see a need to bring these back to
2.4. My guess is that there is no more churn in the common code on
master than with the Windows patches here.

So please just target 2.5 as the release to make an announcement about
Hyper-V support. I promise that this release cycle won't be as long as
2.4.
Nithin Raju Sept. 23, 2015, 3:38 p.m. UTC | #10
> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:

> 

> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com> wrote:

>> hi Jesse,

>> We are getting the Hyper-V solution to a state with the following goals:

>> - Work “out of the box” ie. no need to make special settings such as disabling checksum offload, TSO, etc.

>> - Reasonably stable

>> 

>> Most of the patches we have checked in so far into 2.4 are geared towards these two goals. Once all of the required changes go in, and we are reasonably confident about the stability, we can hopefully make an announcement about Hyper-V support.

> 

> This is my concern - there should not be any announcements based on

> stable branches because there should be no development on stable

> branches. The only thing that should go in is targeted bug fixes to

> address issues that came up after the release.

> 

> All of the goals that you listed are good things and make sense - on

> the master branch. However, I don't see a need to bring these back to

> 2.4. My guess is that there is no more churn in the common code on

> master than with the Windows patches here.

> 

> So please just target 2.5 as the release to make an announcement about

> Hyper-V support. I promise that this release cycle won't be as long as

> 2.4.


Jesse,
We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to announce support. Would that not be the right release vehicle?

thanks,
-- Nithin
Jesse Gross Sept. 23, 2015, 3:49 p.m. UTC | #11
On Wed, Sep 23, 2015 at 8:38 AM, Nithin Raju <nithin@vmware.com> wrote:
>> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:
>>
>> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com> wrote:
>>> hi Jesse,
>>> We are getting the Hyper-V solution to a state with the following goals:
>>> - Work “out of the box” ie. no need to make special settings such as disabling checksum offload, TSO, etc.
>>> - Reasonably stable
>>>
>>> Most of the patches we have checked in so far into 2.4 are geared towards these two goals. Once all of the required changes go in, and we are reasonably confident about the stability, we can hopefully make an announcement about Hyper-V support.
>>
>> This is my concern - there should not be any announcements based on
>> stable branches because there should be no development on stable
>> branches. The only thing that should go in is targeted bug fixes to
>> address issues that came up after the release.
>>
>> All of the goals that you listed are good things and make sense - on
>> the master branch. However, I don't see a need to bring these back to
>> 2.4. My guess is that there is no more churn in the common code on
>> master than with the Windows patches here.
>>
>> So please just target 2.5 as the release to make an announcement about
>> Hyper-V support. I promise that this release cycle won't be as long as
>> 2.4.
>
> Jesse,
> We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to announce support. Would that not be the right release vehicle?

No, there should be no new features in point releases. 2.5 is the next
release where it would makes sense to do this. My guess is that will
be some time around the end of the year.
Ben Pfaff Sept. 29, 2015, 5:55 p.m. UTC | #12
On Wed, Sep 23, 2015 at 08:49:49AM -0700, Jesse Gross wrote:
> On Wed, Sep 23, 2015 at 8:38 AM, Nithin Raju <nithin@vmware.com> wrote:
> >> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:
> >>
> >> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com> wrote:
> >>> hi Jesse,
> >>> We are getting the Hyper-V solution to a state with the following goals:
> >>> - Work “out of the box” ie. no need to make special settings such as disabling checksum offload, TSO, etc.
> >>> - Reasonably stable
> >>>
> >>> Most of the patches we have checked in so far into 2.4 are geared towards these two goals. Once all of the required changes go in, and we are reasonably confident about the stability, we can hopefully make an announcement about Hyper-V support.
> >>
> >> This is my concern - there should not be any announcements based on
> >> stable branches because there should be no development on stable
> >> branches. The only thing that should go in is targeted bug fixes to
> >> address issues that came up after the release.
> >>
> >> All of the goals that you listed are good things and make sense - on
> >> the master branch. However, I don't see a need to bring these back to
> >> 2.4. My guess is that there is no more churn in the common code on
> >> master than with the Windows patches here.
> >>
> >> So please just target 2.5 as the release to make an announcement about
> >> Hyper-V support. I promise that this release cycle won't be as long as
> >> 2.4.
> >
> > Jesse,
> > We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to announce support. Would that not be the right release vehicle?
> 
> No, there should be no new features in point releases. 2.5 is the next
> release where it would makes sense to do this. My guess is that will
> be some time around the end of the year.

The patches I've seen from the Hyper-V developers so far are just in
Hyper-V specific code, that can't really affect the stability of the
rest of the platform.  I have questions about the value of doing this on
2.4, given that 2.5 will branch in a reasonable amount of time, but
since they're eager to do it I'm not sure that it's worth discouraging
them ;-)
Jesse Gross Sept. 29, 2015, 8:05 p.m. UTC | #13
On Tuesday, September 29, 2015, Ben Pfaff <blp@nicira.com
<javascript:_e(%7B%7D,'cvml','blp@nicira.com');>> wrote:

> On Wed, Sep 23, 2015 at 08:49:49AM -0700, Jesse Gross wrote:
> > On Wed, Sep 23, 2015 at 8:38 AM, Nithin Raju <nithin@vmware.com> wrote:
> > >> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:
> > >>
> > >> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com>
> wrote:
> > >>> hi Jesse,
> > >>> We are getting the Hyper-V solution to a state with the following
> goals:
> > >>> - Work “out of the box” ie. no need to make special settings such as
> disabling checksum offload, TSO, etc.
> > >>> - Reasonably stable
> > >>>
> > >>> Most of the patches we have checked in so far into 2.4 are geared
> towards these two goals. Once all of the required changes go in, and we are
> reasonably confident about the stability, we can hopefully make an
> announcement about Hyper-V support.
> > >>
> > >> This is my concern - there should not be any announcements based on
> > >> stable branches because there should be no development on stable
> > >> branches. The only thing that should go in is targeted bug fixes to
> > >> address issues that came up after the release.
> > >>
> > >> All of the goals that you listed are good things and make sense - on
> > >> the master branch. However, I don't see a need to bring these back to
> > >> 2.4. My guess is that there is no more churn in the common code on
> > >> master than with the Windows patches here.
> > >>
> > >> So please just target 2.5 as the release to make an announcement about
> > >> Hyper-V support. I promise that this release cycle won't be as long as
> > >> 2.4.
> > >
> > > Jesse,
> > > We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to
> announce support. Would that not be the right release vehicle?
> >
> > No, there should be no new features in point releases. 2.5 is the next
> > release where it would makes sense to do this. My guess is that will
> > be some time around the end of the year.
>
> The patches I've seen from the Hyper-V developers so far are just in
> Hyper-V specific code, that can't really affect the stability of the
> rest of the platform.  I have questions about the value of doing this on
> 2.4, given that 2.5 will branch in a reasonable amount of time, but
> since they're eager to do it I'm not sure that it's worth discouraging
> them ;-)
>

I think it does cause problems for people who want to upgrade OVS in
production deployments as often it is not desirable to just take a large
set of changes wholesale for a bugfix release. If there are many unrelated
commits for feature development it makes it harder to identify what is
actually going on. I'm also not sure that it is true that there are no
changes to common code.
Ben Pfaff Sept. 30, 2015, 12:01 a.m. UTC | #14
On Tue, Sep 29, 2015 at 01:05:33PM -0700, Jesse Gross wrote:
> On Tuesday, September 29, 2015, Ben Pfaff <blp@nicira.com
> <javascript:_e(%7B%7D,'cvml','blp@nicira.com');>> wrote:
> 
> > On Wed, Sep 23, 2015 at 08:49:49AM -0700, Jesse Gross wrote:
> > > On Wed, Sep 23, 2015 at 8:38 AM, Nithin Raju <nithin@vmware.com> wrote:
> > > >> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:
> > > >>
> > > >> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com>
> > wrote:
> > > >>> hi Jesse,
> > > >>> We are getting the Hyper-V solution to a state with the following
> > goals:
> > > >>> - Work “out of the box” ie. no need to make special settings such as
> > disabling checksum offload, TSO, etc.
> > > >>> - Reasonably stable
> > > >>>
> > > >>> Most of the patches we have checked in so far into 2.4 are geared
> > towards these two goals. Once all of the required changes go in, and we are
> > reasonably confident about the stability, we can hopefully make an
> > announcement about Hyper-V support.
> > > >>
> > > >> This is my concern - there should not be any announcements based on
> > > >> stable branches because there should be no development on stable
> > > >> branches. The only thing that should go in is targeted bug fixes to
> > > >> address issues that came up after the release.
> > > >>
> > > >> All of the goals that you listed are good things and make sense - on
> > > >> the master branch. However, I don't see a need to bring these back to
> > > >> 2.4. My guess is that there is no more churn in the common code on
> > > >> master than with the Windows patches here.
> > > >>
> > > >> So please just target 2.5 as the release to make an announcement about
> > > >> Hyper-V support. I promise that this release cycle won't be as long as
> > > >> 2.4.
> > > >
> > > > Jesse,
> > > > We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to
> > announce support. Would that not be the right release vehicle?
> > >
> > > No, there should be no new features in point releases. 2.5 is the next
> > > release where it would makes sense to do this. My guess is that will
> > > be some time around the end of the year.
> >
> > The patches I've seen from the Hyper-V developers so far are just in
> > Hyper-V specific code, that can't really affect the stability of the
> > rest of the platform.  I have questions about the value of doing this on
> > 2.4, given that 2.5 will branch in a reasonable amount of time, but
> > since they're eager to do it I'm not sure that it's worth discouraging
> > them ;-)
> >
> 
> I think it does cause problems for people who want to upgrade OVS in
> production deployments as often it is not desirable to just take a large
> set of changes wholesale for a bugfix release. If there are many unrelated
> commits for feature development it makes it harder to identify what is
> actually going on. I'm also not sure that it is true that there are no
> changes to common code.

Nithin, Alin, et al., why do you guys want this on branch-2.4 so badly?
Everything that Jesse is saying is right, and despite my initial
instincts I'm inclined to agree with him in the end.
Alin Serdean Sept. 30, 2015, 9:22 p.m. UTC | #15
> > >

> > > The patches I've seen from the Hyper-V developers so far are just in

> > > Hyper-V specific code, that can't really affect the stability of the

> > > rest of the platform.  I have questions about the value of doing

> > > this on 2.4, given that 2.5 will branch in a reasonable amount of

> > > time, but since they're eager to do it I'm not sure that it's worth

> > > discouraging them ;-)

> > >

> >

> > I think it does cause problems for people who want to upgrade OVS in

> > production deployments as often it is not desirable to just take a

> > large set of changes wholesale for a bugfix release. If there are many

> > unrelated commits for feature development it makes it harder to

> > identify what is actually going on. I'm also not sure that it is true

> > that there are no changes to common code.

> 

> Nithin, Alin, et al., why do you guys want this on branch-2.4 so badly?

> Everything that Jesse is saying is right, and despite my initial instincts I'm

> inclined to agree with him in the end.

[Alin Gabriel Serdean: ] We would like to encourage users to start using OVS under Hyper-V so we can get as much feedback as possible. 
Making it more easy to use it straight out of the box and under a release version would help us in this matter.

Alin.
Nithin Raju Oct. 1, 2015, 2:46 a.m. UTC | #16
> On Sep 29, 2015, at 5:01 PM, Ben Pfaff <blp@nicira.com> wrote:

> 

> On Tue, Sep 29, 2015 at 01:05:33PM -0700, Jesse Gross wrote:

>> On Tuesday, September 29, 2015, Ben Pfaff <blp@nicira.com

>> <javascript:_e(%7B%7D,'cvml','blp@nicira.com');>> wrote:

>> 

>>> On Wed, Sep 23, 2015 at 08:49:49AM -0700, Jesse Gross wrote:

>>>> On Wed, Sep 23, 2015 at 8:38 AM, Nithin Raju <nithin@vmware.com> wrote:

>>>>>> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:

>>>>>> 

>>>>>> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com>

>>> wrote:

>>>>>>> hi Jesse,

>>>>>>> We are getting the Hyper-V solution to a state with the following

>>> goals:

>>>>>>> - Work “out of the box” ie. no need to make special settings such as

>>> disabling checksum offload, TSO, etc.

>>>>>>> - Reasonably stable

>>>>>>> 

>>>>>>> Most of the patches we have checked in so far into 2.4 are geared

>>> towards these two goals. Once all of the required changes go in, and we are

>>> reasonably confident about the stability, we can hopefully make an

>>> announcement about Hyper-V support.

>>>>>> 

>>>>>> This is my concern - there should not be any announcements based on

>>>>>> stable branches because there should be no development on stable

>>>>>> branches. The only thing that should go in is targeted bug fixes to

>>>>>> address issues that came up after the release.

>>>>>> 

>>>>>> All of the goals that you listed are good things and make sense - on

>>>>>> the master branch. However, I don't see a need to bring these back to

>>>>>> 2.4. My guess is that there is no more churn in the common code on

>>>>>> master than with the Windows patches here.

>>>>>> 

>>>>>> So please just target 2.5 as the release to make an announcement about

>>>>>> Hyper-V support. I promise that this release cycle won't be as long as

>>>>>> 2.4.

>>>>> 

>>>>> Jesse,

>>>>> We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to

>>> announce support. Would that not be the right release vehicle?

>>>> 

>>>> No, there should be no new features in point releases. 2.5 is the next

>>>> release where it would makes sense to do this. My guess is that will

>>>> be some time around the end of the year.

>>> 

>>> The patches I've seen from the Hyper-V developers so far are just in

>>> Hyper-V specific code, that can't really affect the stability of the

>>> rest of the platform.  I have questions about the value of doing this on

>>> 2.4, given that 2.5 will branch in a reasonable amount of time, but

>>> since they're eager to do it I'm not sure that it's worth discouraging

>>> them ;-)

>>> 

>> 

>> I think it does cause problems for people who want to upgrade OVS in

>> production deployments as often it is not desirable to just take a large

>> set of changes wholesale for a bugfix release. If there are many unrelated

>> commits for feature development it makes it harder to identify what is

>> actually going on. I'm also not sure that it is true that there are no

>> changes to common code.

> 

> Nithin, Alin, et al., why do you guys want this on branch-2.4 so badly?

> Everything that Jesse is saying is right, and despite my initial

> instincts I'm inclined to agree with him in the end.


Ben,
Our goal is to get onto the next earliest release of OVS be it 2.5 or 2.4.1.

If you think that 2.5 will happen soon enough, we can wait for it. Otherwise, we are inclined to request a 2.4.1 release with Hyper-V support.

For now, we can commit to master, and perhaps decide in a month or so as to what the release branch should be. If it turns out to be 2.4.1, we’ll have to do a bunch of crossposts, and I can help with that.

Is that reasonable?

thanks,
-- Nithin
Ben Pfaff Oct. 2, 2015, 2:13 p.m. UTC | #17
On Thu, Oct 01, 2015 at 02:46:32AM +0000, Nithin Raju wrote:
> > On Sep 29, 2015, at 5:01 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > On Tue, Sep 29, 2015 at 01:05:33PM -0700, Jesse Gross wrote:
> >> On Tuesday, September 29, 2015, Ben Pfaff <blp@nicira.com
> >> <javascript:_e(%7B%7D,'cvml','blp@nicira.com');>> wrote:
> >> 
> >>> On Wed, Sep 23, 2015 at 08:49:49AM -0700, Jesse Gross wrote:
> >>>> On Wed, Sep 23, 2015 at 8:38 AM, Nithin Raju <nithin@vmware.com> wrote:
> >>>>>> On Sep 23, 2015, at 8:33 AM, Jesse Gross <jesse@nicira.com> wrote:
> >>>>>> 
> >>>>>> On Tue, Sep 22, 2015 at 11:04 PM, Nithin Raju <nithin@vmware.com>
> >>> wrote:
> >>>>>>> hi Jesse,
> >>>>>>> We are getting the Hyper-V solution to a state with the following
> >>> goals:
> >>>>>>> - Work “out of the box” ie. no need to make special settings such as
> >>> disabling checksum offload, TSO, etc.
> >>>>>>> - Reasonably stable
> >>>>>>> 
> >>>>>>> Most of the patches we have checked in so far into 2.4 are geared
> >>> towards these two goals. Once all of the required changes go in, and we are
> >>> reasonably confident about the stability, we can hopefully make an
> >>> announcement about Hyper-V support.
> >>>>>> 
> >>>>>> This is my concern - there should not be any announcements based on
> >>>>>> stable branches because there should be no development on stable
> >>>>>> branches. The only thing that should go in is targeted bug fixes to
> >>>>>> address issues that came up after the release.
> >>>>>> 
> >>>>>> All of the goals that you listed are good things and make sense - on
> >>>>>> the master branch. However, I don't see a need to bring these back to
> >>>>>> 2.4. My guess is that there is no more churn in the common code on
> >>>>>> master than with the Windows patches here.
> >>>>>> 
> >>>>>> So please just target 2.5 as the release to make an announcement about
> >>>>>> Hyper-V support. I promise that this release cycle won't be as long as
> >>>>>> 2.4.
> >>>>> 
> >>>>> Jesse,
> >>>>> We were hoping for a dot release off of 2.4. Like a 2.4.1 or 2.4.2 to
> >>> announce support. Would that not be the right release vehicle?
> >>>> 
> >>>> No, there should be no new features in point releases. 2.5 is the next
> >>>> release where it would makes sense to do this. My guess is that will
> >>>> be some time around the end of the year.
> >>> 
> >>> The patches I've seen from the Hyper-V developers so far are just in
> >>> Hyper-V specific code, that can't really affect the stability of the
> >>> rest of the platform.  I have questions about the value of doing this on
> >>> 2.4, given that 2.5 will branch in a reasonable amount of time, but
> >>> since they're eager to do it I'm not sure that it's worth discouraging
> >>> them ;-)
> >>> 
> >> 
> >> I think it does cause problems for people who want to upgrade OVS in
> >> production deployments as often it is not desirable to just take a large
> >> set of changes wholesale for a bugfix release. If there are many unrelated
> >> commits for feature development it makes it harder to identify what is
> >> actually going on. I'm also not sure that it is true that there are no
> >> changes to common code.
> > 
> > Nithin, Alin, et al., why do you guys want this on branch-2.4 so badly?
> > Everything that Jesse is saying is right, and despite my initial
> > instincts I'm inclined to agree with him in the end.
> 
> Ben,
> Our goal is to get onto the next earliest release of OVS be it 2.5 or 2.4.1.
> 
> If you think that 2.5 will happen soon enough, we can wait for it. Otherwise, we are inclined to request a 2.4.1 release with Hyper-V support.
> 
> For now, we can commit to master, and perhaps decide in a month or so as to what the release branch should be. If it turns out to be 2.4.1, we’ll have to do a bunch of crossposts, and I can help with that.
> 
> Is that reasonable?

I think it's best to wait to branch 2.5.

Patch
diff mbox

diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
index b6272c3..4a5a4a6 100644
--- a/datapath-windows/ovsext/Stt.c
+++ b/datapath-windows/ovsext/Stt.c
@@ -146,10 +146,16 @@  OvsDoEncapStt(POVS_VPORT_ENTRY vport,
     POVS_STT_VPORT vportStt;
     UINT32 headRoom = OvsGetSttTunHdrSize();
     UINT32 tcpChksumLen;
+    PUINT8 bufferStart;
 
     UNREFERENCED_PARAMETER(layers);
 
     curNb = NET_BUFFER_LIST_FIRST_NB(curNbl);
+
+    /* Verify if inner checksum is verified */
+    BOOLEAN innerChecksumVerified = FALSE;
+    BOOLEAN innerPartialChecksum = FALSE;
+
     if (layers->isTcp) {
         NDIS_TCP_LARGE_SEND_OFFLOAD_NET_BUFFER_LIST_INFO lsoInfo;
 
@@ -165,6 +171,9 @@  OvsDoEncapStt(POVS_VPORT_ENTRY vport,
     vportStt = (POVS_STT_VPORT) GetOvsVportPriv(vport);
     ASSERT(vportStt);
 
+    NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
+    csumInfo.Value = NET_BUFFER_LIST_INFO(curNbl,
+                                          TcpIpChecksumNetBufferListInfo);
     *newNbl = OvsPartialCopyNBL(switchContext, curNbl, 0, headRoom,
                                 FALSE /*copy NblInfo*/);
     if (*newNbl == NULL) {
@@ -172,6 +181,30 @@  OvsDoEncapStt(POVS_VPORT_ENTRY vport,
         return NDIS_STATUS_FAILURE;
     }
 
+    curNb = NET_BUFFER_LIST_FIRST_NB(*newNbl);
+    curMdl = NET_BUFFER_CURRENT_MDL(curNb);
+    bufferStart = (PUINT8)MmGetSystemAddressForMdlSafe(curMdl,
+                                                       LowPagePriority);
+    bufferStart += NET_BUFFER_CURRENT_MDL_OFFSET(curNb);
+
+    if (layers->isIPv4 && csumInfo.Transmit.IpHeaderChecksum) {
+        IPHdr *ip = (IPHdr *)(bufferStart + layers->l3Offset);
+        ip->check = IPChecksum((UINT8 *)ip, ip->ihl * 4, 0);
+    }
+    if (layers->isTcp) {
+        if(!csumInfo.Transmit.TcpChecksum) {
+            innerChecksumVerified = TRUE;
+        } else {
+            innerPartialChecksum = TRUE;
+        }
+    } else if (layers->isUdp) {
+        if(!csumInfo.Transmit.UdpChecksum) {
+            innerChecksumVerified = TRUE;
+        } else {
+            innerPartialChecksum = TRUE;
+        }
+    }
+
     curNbl = *newNbl;
     curNb = NET_BUFFER_LIST_FIRST_NB(curNbl);
     /* NB Chain should be split before */
@@ -243,7 +276,6 @@  OvsDoEncapStt(POVS_VPORT_ENTRY vport,
     outerIpHdr->check = 0;
     outerIpHdr->saddr = fwdInfo->srcIpAddr;
     outerIpHdr->daddr = tunKey->dst;
-    outerIpHdr->check = IPChecksum((uint8 *)outerIpHdr, sizeof *outerIpHdr, 0);
 
     /* L4 header */
     RtlZeroMemory(outerTcpHdr, sizeof *outerTcpHdr);
@@ -266,6 +298,11 @@  OvsDoEncapStt(POVS_VPORT_ENTRY vport,
 
     /* XXX need to peek into the inner packet, hard code for now */
     sttHdr->flags = STT_PROTO_IPV4;
+    if (innerChecksumVerified) {
+        sttHdr->flags |= STT_CSUM_VERIFIED;
+    } else if (innerPartialChecksum) {
+        sttHdr->flags |= STT_CSUM_PARTIAL;
+    }
     sttHdr->l4Offset = 0;
 
     sttHdr->reserved = 0;
@@ -276,13 +313,15 @@  OvsDoEncapStt(POVS_VPORT_ENTRY vport,
     /* Zero out stt padding */
     *(uint16 *)(sttHdr + 1) = 0;
 
-    /* Calculate software tcp checksum */
-    outerTcpHdr->check = CalculateChecksumNB(curNb, (uint16) tcpChksumLen,
-                                             sizeof(EthHdr) + sizeof(IPHdr));
-    if (outerTcpHdr->check == 0) {
-        status = NDIS_STATUS_FAILURE;
-        goto ret_error;
-    }
+    /* Offload IP and TCP checksum */
+    csumInfo.Value = 0;
+    csumInfo.Transmit.IpHeaderChecksum = 1;
+    csumInfo.Transmit.TcpChecksum = 1;
+    csumInfo.Transmit.IsIPv4 = 1;
+    csumInfo.Transmit.TcpHeaderOffset = sizeof *outerEthHdr +
+                                        outerIpHdr->ihl * 4;
+    NET_BUFFER_LIST_INFO(curNbl,
+                         TcpIpChecksumNetBufferListInfo) = csumInfo.Value;
 
     return STATUS_SUCCESS;
 
@@ -293,6 +332,53 @@  ret_error:
 }
 
 /*
+ *----------------------------------------------------------------------------
+ * OvsCalculateTCPChecksum
+ *     Calculate TCP checksum
+ *----------------------------------------------------------------------------
+ */
+static __inline NDIS_STATUS
+OvsCalculateTCPChecksum(PNET_BUFFER_LIST curNbl, PNET_BUFFER curNb)
+{
+    NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
+    csumInfo.Value = NET_BUFFER_LIST_INFO(curNbl,
+                                          TcpIpChecksumNetBufferListInfo);
+    UINT16 checkSum;
+
+    /* Check if TCP Checksum has been calculated by NIC */
+    if (csumInfo.Receive.TcpChecksumSucceeded) {
+        return NDIS_STATUS_SUCCESS;
+    }
+        
+    EthHdr *eth = (EthHdr *)NdisGetDataBuffer(curNb, sizeof(EthHdr),
+                                              NULL, 1, 0);
+
+    if (eth->Type == ntohs(NDIS_ETH_TYPE_IPV4)) {
+        IPHdr *ip = (IPHdr *)((PCHAR)eth + sizeof *eth);
+        UINT32 l4Payload = ntohs(ip->tot_len) - ip->ihl * 4;
+        TCPHdr *tcp = (TCPHdr *)((PCHAR)ip + ip->ihl * 4);
+        checkSum = tcp->check;
+
+        tcp->check = 0;
+        tcp->check = IPPseudoChecksum(&ip->saddr, &ip->daddr,
+                                      IPPROTO_TCP, (UINT16)l4Payload);
+        tcp->check = CalculateChecksumNB(curNb, (UINT16)(l4Payload),
+                                         sizeof(EthHdr) + ip->ihl * 4);
+        if (checkSum != tcp->check) {
+            return NDIS_STATUS_INVALID_PACKET;
+        }
+    } else {
+        OVS_LOG_ERROR("IPv6 on STT is not supported");
+        return NDIS_STATUS_INVALID_PACKET;
+    }
+
+    csumInfo.Receive.TcpChecksumSucceeded = 1;
+    NET_BUFFER_LIST_INFO(curNbl,
+                         TcpIpChecksumNetBufferListInfo) = csumInfo.Value;
+    return NDIS_STATUS_SUCCESS;
+}
+
+/*
  * --------------------------------------------------------------------------
  * OvsDecapStt --
  *     Decapsulates an STT packet.
@@ -311,6 +397,7 @@  OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
     SttHdr *sttHdr;
     char *sttBuf[STT_HDR_LEN];
     UINT32 advanceCnt, hdrLen;
+    NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO csumInfo;
 
     curNb = NET_BUFFER_LIST_FIRST_NB(curNbl);
     ASSERT(NET_BUFFER_NEXT_NB(curNb) == NULL);
@@ -321,6 +408,21 @@  OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
         return NDIS_STATUS_INVALID_LENGTH;
     }
 
+    /* Verify outer TCP Checksum */
+    csumInfo.Value = NET_BUFFER_LIST_INFO(curNbl,
+                                          TcpIpChecksumNetBufferListInfo);
+
+    /* Check if NIC has indicated TCP checksum failure */
+    if (csumInfo.Receive.TcpChecksumFailed) {
+        return NDIS_STATUS_INVALID_PACKET;
+    }
+    
+    /* Calculate the TCP Checksum */
+    status = OvsCalculateTCPChecksum(curNbl, curNb);
+    if (status != NDIS_STATUS_SUCCESS) {
+        return status;
+    }
+
     /* Skip Eth header */
     hdrLen = sizeof(EthHdr);
     NdisAdvanceNetBufferDataStart(curNb, hdrLen, FALSE, NULL);
@@ -353,6 +455,61 @@  OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
     hdrLen = STT_HDR_LEN;
     NdisAdvanceNetBufferDataStart(curNb, hdrLen, FALSE, NULL);
     advanceCnt += hdrLen;
+    
+    /* Verify checksum for inner packet if it's required */
+    if (!(sttHdr->flags & STT_CSUM_VERIFIED)) {
+        BOOLEAN innerChecksumPartial = sttHdr->flags & STT_CSUM_PARTIAL;
+        EthHdr *eth = (EthHdr *)NdisGetDataBuffer(curNb, sizeof(EthHdr),
+                                                  NULL, 1, 0);
+
+        /* XXX Figure out a way to offload checksum receives */
+        if (eth->Type == ntohs(NDIS_ETH_TYPE_IPV4)) {
+            IPHdr *ip = (IPHdr *)((PCHAR)eth + sizeof *eth);
+            UINT16 l4Payload = (UINT16)ntohs(ip->tot_len) - ip->ihl * 4;
+            UINT32 offset = sizeof(EthHdr) + ip->ihl * 4;
+
+            if (ip->protocol == IPPROTO_TCP) {
+                TCPHdr *tcp = (TCPHdr *)((PCHAR)ip + ip->ihl * 4);
+                if (!innerChecksumPartial){
+                    tcp->check = IPPseudoChecksum(&ip->saddr, &ip->daddr,
+                                                  IPPROTO_TCP,
+                                                  (UINT16)l4Payload);
+                }
+                tcp->check = CalculateChecksumNB(curNb, l4Payload, offset);
+            } else if (ip->protocol == IPPROTO_UDP) {
+                UDPHdr *udp = (UDPHdr *)((PCHAR)ip + sizeof *ip);
+                if (!innerChecksumPartial){
+                    udp->check = IPPseudoChecksum(&ip->saddr, &ip->daddr,
+                                                  IPPROTO_UDP, l4Payload);
+                }
+                udp->check = CalculateChecksumNB(curNb, l4Payload, offset);
+            }
+        } else if (eth->Type == ntohs(NDIS_ETH_TYPE_IPV6)) {
+            IPv6Hdr *ip = (IPv6Hdr *)((PCHAR)eth + sizeof *eth);
+            UINT32 offset = (UINT32)(sizeof *eth + sizeof *ip);
+            UINT16 totalLength = (UINT16)ntohs(ip->payload_len);
+            if (ip->nexthdr == IPPROTO_TCP) {
+                TCPHdr *tcp = (TCPHdr *)((PCHAR)ip + sizeof *ip);
+                if (!innerChecksumPartial){
+                    tcp->check = IPv6PseudoChecksum((UINT32 *)&ip->saddr,
+                                                    (UINT32 *)&ip->daddr,
+                                                    IPPROTO_TCP, totalLength);
+                }
+                tcp->check = CalculateChecksumNB(curNb, totalLength, offset);
+            }
+            else if (ip->nexthdr == IPPROTO_UDP) {
+                UDPHdr *udp = (UDPHdr *)((PCHAR)ip + sizeof *ip);
+                if (!innerChecksumPartial) {
+                    udp->check = IPv6PseudoChecksum((UINT32 *)&ip->saddr,
+                                                    (UINT32 *)&ip->daddr,
+                                                    IPPROTO_UDP, totalLength);
+                }
+                udp->check = CalculateChecksumNB(curNb, totalLength, offset);
+            }
+        }
+
+        NET_BUFFER_LIST_INFO(curNbl, TcpIpChecksumNetBufferListInfo) = 0;
+    }
 
     *newNbl = OvsPartialCopyNBL(switchContext, curNbl, OVS_DEFAULT_COPY_SIZE,
                                 0, FALSE /*copy NBL info*/);