Message-ID: <d362d083-e0b2-44fe-8bbb-5a0286ace230@intel.com>
Date: Wed, 3 Jul 2024 17:02:32 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Tom Herbert <tom@...bertland.com>, "Greenwalt, Paul"
	<paul.greenwalt@...el.com>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC: "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>, <cai.huoqing@...ux.dev>, Linux
 Kernel Network Developers <netdev@...r.kernel.org>, "Felipe Magno de Almeida"
	<felipe@...anda.io>, Justin Iurman <justin.iurman@...ege.be>, Alexander
 Lobakin <aleksander.lobakin@...el.com>, "Samudrala, Sridhar"
	<sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next v2 0/7] drivers: Fix drivers doing TX csum
 offload with EH

On 7/3/24 16:38, Tom Herbert wrote:
> 
> 
> On Wed, Jul 3, 2024, 7:20 AM Greenwalt, Paul <paul.greenwalt@...el.com 
> <mailto:paul.greenwalt@...el.com>> wrote:
> 
> 
> 
>     On 7/2/2024 3:31 AM, Przemek Kitszel wrote:
>      > On 7/1/24 21:55, Tom Herbert wrote:
>      >> Several NICs would seem to support protocol specific TX checksum
>     offload
>      >> and allow for cases where an IPv6 packet contains extension headers.
>      >> When deciding whether to offload a packet, ipv6_skip_exthdr is
>     called
>      >> to skip extension headers. The problem is that if a packet
>     contains an
>      >> IPv6 Routing Header then protocol specific checksum offload
>     can't work,
>      >> the destination IP address in the IPv6 header is not the same
>     one that
>      >> is used in the pseudo header for TCP or UDP. The correct address is
>      >> derived from the last segment in the routing list (which itself
>     might
>      >> be obfuscated so that a device could even read it).
>      >
>      > feels like there is a missing "not" after "could" - with it
>     added, reads
>      > fine (not a request to change, just being verbose about assumptions)
>      >
>      >>
>      >> This patch set adds a new function ipv6_skip_exthdr_no_rthdr to be
>      >> called in lieu of ipv6_skip_exthdr. If a routing header is
>     present in
>      >> a packet then ipv6_skip_exthdr_no_rthdr returns a value less than
>      >> zero, this is an indication to the driver that TX checksum offload
>      >> is not viable and it should call skb_checksum_help instead of
>      >> offloading the checksum.
>      >>
>      >> The i40e, iavf, ice, idpf, hinic, and fm10k are updated accordingly
>      >> to call ipv6_skip_exthdr_no_rthdr.
>      >>
>      >> Testing: The code compiles, but is otherwise untested due to lack of
>      >> NIC hardware. It would be appreciated if someone with access to the
>      >> hardware could test.
>      >
>      > we could test intel ones (except fm10k) via @Tony's tree
> 
> 
> Awesome! If you need any help let me know.
> 
>      >
>      >>
>      >> v2: Fixed uninitialized variable in exthdrs_core.c
>      >>
>      >> Tom Herbert (7):
>      >>    ipv6: Add ipv6_skip_exthdr_no_rthdr
>      >>    i40e: Don't do TX csum offload with routing header present
>      >>    iavf: Don't do TX csum offload with routing header present
>      >>    ice: Don't do TX csum offload with routing header present
>      >
>      > sidenote:
>      > our HW is supporting (among others) a GCO check-summing mode
>     described
>      > as: "Checksum 16bit (TCP/UDP) with no pseudo Header", but we have not
>      > yet provided patches for that, and I don't even know if this mode
>      > will be used (CC @Paul)
>      >
> 
>     We will be adding support for GCO "Checksum 16 with pseudo Headers" to
>     the ice driver. It will be off by default.
> 
> 
> I'm not sure what that means. 

IPv6 Routing Headers render (simple?) HW-offloaded checksumming wrong,
but not for the "no pseudo Header"-checksum. I have no idea how such
checksum will be useful, and we don't have plans to implement it, so
this is not much relevant. But that is just one mode that you could 
config our (new) HW.

> Can ICE just provide checksum-complete? 
> It's by far the simplest, most robust method with the most flexibility 
> for users. :-)

sorry, could you please elaborate?

Paul will implement GCO for ice and otherwise my understanding was that
our checksum is fine. Is there a room for improvement?

> 
> Tom
> 
> 
>      >>    idpf: Don't do TX csum offload with routing header present
>      >>    hinic: Don't do TX csum offload with routing header present
>      >>    fm10k: Don't do TX csum offload with routing header present
>      >>
>      >>   drivers/net/ethernet/huawei/hinic/hinic_tx.c  | 23 +++++++++++----
>      >>   drivers/net/ethernet/intel/fm10k/fm10k_main.c |  9 ++++--
>      >>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 22 ++++++---------
>      >>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 20 ++++++-------
>      >>   drivers/net/ethernet/intel/ice/ice_txrx.c     | 22 ++++++---------
>      >>   .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 28
>     +++++++++----------
>      >>   include/net/ipv6.h                            | 17 +++++++++--
>      >>   net/ipv6/exthdrs_core.c                       | 25
>     ++++++++++++-----
>      >>   8 files changed, 98 insertions(+), 68 deletions(-)
>      >>
>      >
>      > I have reviewed the patches and they conform to commit
>     message/intent,
>      > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com
>     <mailto:przemyslaw.kitszel@...el.com>>
>      > (for the series)
> 

Powered by blists - more mailing lists