On 30.07.2018 17:40, Jerin Jacob wrote:

-----Original Message-----

quoted

Date: Mon, 30 Jul 2018 14:12:12 +0000
From: "Ananyev, Konstantin" <redacted>
To: Jerin Jacob <redacted>
CC: Thomas Monjalon <redacted>, "dev@dpdk.org" <redacted>,
  "Yigit, Ferruh" [off-list ref], "shahafs@mellanox.com"
  [off-list ref]
Subject: RE: [dpdk-dev] [PATCH] examples: remove Rx checksum offload

quoted

-----Original Message-----

quoted

Date: Mon, 30 Jul 2018 11:00:02 +0000
From: "Ananyev, Konstantin" <redacted>
To: Thomas Monjalon <redacted>, Jerin Jacob
  [off-list ref]
CC: "dev@dpdk.org" <redacted>, "Yigit, Ferruh"
  [off-list ref], "shahafs@mellanox.com" [off-list ref]
Subject: RE: [dpdk-dev] [PATCH] examples: remove Rx checksum offload

External Email

quoted

-----Original Message-----
From: Thomas Monjalon [mailto:thomas@monjalon.net]
Sent: Monday, July 30, 2018 10:51 AM
To: Jerin Jacob <redacted>; Ananyev, Konstantin <redacted>
Cc: dev@dpdk.org; Yigit, Ferruh <redacted>; shahafs@mellanox.com
Subject: Re: [dpdk-dev] [PATCH] examples: remove Rx checksum offload

30/07/2018 11:35, Jerin Jacob:

quoted

From: "Ananyev, Konstantin" <redacted>

quoted

quoted

As of now, application does not check PKT_RX_*_CKSUM_* flags per
packet, so it does not matter DEV_RX_OFFLOAD_CHECKSUM enabled or not.

Removing DEV_RX_OFFLOAD_CHECKSUM offload so that driver can save a few
cycles if possible.

Personally, I'd move in other direction: keep RX checksum offload and add
checks inside sample apps to handle (drop) packets with invalid checksum.

OK. Till someones add the DROP logic in application, Can we take
this patch? Because there is no point in enabling DEV_RX_OFFLOAD_CHECKSUM
without DROP or any meaning full action in application.

Probably, but at least it gives users a right estimation how long the proper
RX/TX routine would take.

For estimation, application can add any flag they want in local setup.
It does not need to be upstream with out feature complete.

quoted

 From other side what the point to disable these flags now, if we know that

At least nicvf Rx routines are crafted based DEV_RX_OFFLOAD_CHECKSUM
flags. If driver Rx routine crafted such case it will be useful.

quoted

we are doing wrong thing and will have to re-enable them again in future?

But it is not correct now either. Right?

Yes, right now invalid cksum information is simply ignored.
As you pointed - some PMD select RX routine based on checksum offload flags.
Yes, removing these flags might produce better performance numbers.
But from my perspective - it would be an artificial and temporary improvement,
as for l3fwd like apps we'll need to revert it back and add code to drop invalid packets.

IMO, It is OK get a performance hit when do that support in l3fwd. There
is no harm in removing the DEV_RX_OFFLOAD_CHECKSUM flag for now and it
is correct from application perspective.(you are enabling an offload when
you are using it, else don't enable it. I believe, this was philosophy for
enabling Rx/Tx offloads)

Since it is going in circles, I leave decision to ethdev maintainers.

I think that IPv4 checksum offload is essential for l3fwd. So, it should be
enabled and taken into account. I'm not sure about TCP and UDP checksum
offloads. It is not l3fwd business to take a look at upper layers.

In any case, there is no agreement on the patch and it is already RC3 stage
of the release. There is no rush to apply it since it is not a critical 
bug fix.
I agree with Konstantin here.

Andrew

quoted

Konstantin

quoted

quoted

quoted

If there is no patch sent to use this offload on August 1st,
then I will apply this patch to remove the offload request.

Isn't it too late to do such things right now?
We are in RC3 stage and doesn't look like a critical issue.

Yes. We can add it when have we proper fix. Currently, it signaling a wrong
interpretation to application.

quoted

Konstantin