Received-SPF: pass (google.com: domain of linux-kernel+bounces-12087-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 27 Dec 2023 10:43:34 -0500
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Alexander Lobakin <aleksander.lobakin@intel.com>, 
 "David S. Miller" <davem@davemloft.net>, 
 Eric Dumazet <edumazet@google.com>, 
 Jakub Kicinski <kuba@kernel.org>, 
 Paolo Abeni <pabeni@redhat.com>
Cc: Alexander Lobakin <aleksander.lobakin@intel.com>, 
 Maciej Fijalkowski <maciej.fijalkowski@intel.com>, 
 Michal Kubiak <michal.kubiak@intel.com>, 
 Larysa Zaremba <larysa.zaremba@intel.com>, 
 Alexei Starovoitov <ast@kernel.org>, 
 Daniel Borkmann <daniel@iogearbox.net>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>, 
 intel-wired-lan@lists.osuosl.org, 
 netdev@vger.kernel.org, 
 linux-kernel@vger.kernel.org
Message-ID: <658c46269aa52_a33e629442@willemb.c.googlers.com.notmuch>
In-Reply-To: <20231223025554.2316836-2-aleksander.lobakin@intel.com>
References: <20231223025554.2316836-1-aleksander.lobakin@intel.com>
 <20231223025554.2316836-2-aleksander.lobakin@intel.com>
Subject: Re: [PATCH RFC net-next 01/34] idpf: reuse libie's definitions of
 parsed ptype structures
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

Alexander Lobakin wrote:
> idpf's in-kernel parsed ptype structure is almost identical to the one
> used in the previous Intel drivers, which means it can be converted to
> use libie's definitions and even helpers. The only difference is that
> it doesn't use a constant table, rather than one obtained from the
> device.
> Remove the driver counterpart and use libie's helpers for hashes and
> checksums. This slightly optimizes skb fields processing due to faster
> checks.
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> ---
>  drivers/net/ethernet/intel/Kconfig            |   1 +
>  drivers/net/ethernet/intel/idpf/idpf.h        |   2 +-
>  drivers/net/ethernet/intel/idpf/idpf_main.c   |   1 +
>  .../ethernet/intel/idpf/idpf_singleq_txrx.c   |  87 +++++++--------
>  drivers/net/ethernet/intel/idpf/idpf_txrx.c   | 101 ++++++------------
>  drivers/net/ethernet/intel/idpf/idpf_txrx.h   |  88 +--------------
>  .../net/ethernet/intel/idpf/idpf_virtchnl.c   |  54 ++++++----
>  7 files changed, 110 insertions(+), 224 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> index c7da7d05d93e..0db1aa36866e 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -378,6 +378,7 @@ config IDPF
>  	tristate "Intel(R) Infrastructure Data Path Function Support"
>  	depends on PCI_MSI
>  	select DIMLIB
> +	select LIBIE
>  	select PAGE_POOL
>  	select PAGE_POOL_STATS
>  	help
> diff --git a/drivers/net/ethernet/intel/idpf/idpf.h b/drivers/net/ethernet/intel/idpf/idpf.h
> index 0acc125decb3..8342df0f4f3d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf.h
> @@ -385,7 +385,7 @@ struct idpf_vport {
>  	u16 num_rxq_grp;
>  	struct idpf_rxq_group *rxq_grps;
>  	u32 rxq_model;
> -	struct idpf_rx_ptype_decoded rx_ptype_lkup[IDPF_RX_MAX_PTYPE];
> +	struct libie_rx_ptype_parsed rx_ptype_lkup[IDPF_RX_MAX_PTYPE];
>  
>  	struct idpf_adapter *adapter;
>  	struct net_device *netdev;
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index e1febc74cefd..6471158e6f6b 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -7,6 +7,7 @@
>  #define DRV_SUMMARY	"Intel(R) Infrastructure Data Path Function Linux Driver"
>  
>  MODULE_DESCRIPTION(DRV_SUMMARY);
> +MODULE_IMPORT_NS(LIBIE);
>  MODULE_LICENSE("GPL");
>  
>  /**
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> index 8122a0cc97de..e58e08c9997d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> @@ -636,75 +636,64 @@ static bool idpf_rx_singleq_is_non_eop(struct idpf_queue *rxq,
>   * @rxq: Rx ring being processed
>   * @skb: skb currently being received and modified
>   * @csum_bits: checksum bits from descriptor
> - * @ptype: the packet type decoded by hardware
> + * @parsed: the packet type parsed by hardware
>   *
>   * skb->protocol must be set before this function is called
>   */
>  static void idpf_rx_singleq_csum(struct idpf_queue *rxq, struct sk_buff *skb,
> -				 struct idpf_rx_csum_decoded *csum_bits,
> -				 u16 ptype)
> +				 struct idpf_rx_csum_decoded csum_bits,
> +				 struct libie_rx_ptype_parsed parsed)
>  {
> -	struct idpf_rx_ptype_decoded decoded;
>  	bool ipv4, ipv6;
>  
>  	/* check if Rx checksum is enabled */
> -	if (unlikely(!(rxq->vport->netdev->features & NETIF_F_RXCSUM)))
> +	if (!libie_has_rx_checksum(rxq->vport->netdev, parsed))
>  		return;
>  
>  	/* check if HW has decoded the packet and checksum */
> -	if (unlikely(!(csum_bits->l3l4p)))
> +	if (unlikely(!csum_bits.l3l4p))
>  		return;
>  
> -	decoded = rxq->vport->rx_ptype_lkup[ptype];
> -	if (unlikely(!(decoded.known && decoded.outer_ip)))
> +	if (unlikely(parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_L2))
>  		return;
>  
> -	ipv4 = IDPF_RX_PTYPE_TO_IPV(&decoded, IDPF_RX_PTYPE_OUTER_IPV4);
> -	ipv6 = IDPF_RX_PTYPE_TO_IPV(&decoded, IDPF_RX_PTYPE_OUTER_IPV6);
> +	ipv4 = parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_IPV4;
> +	ipv6 = parsed.outer_ip == LIBIE_RX_PTYPE_OUTER_IPV6;
>  
>  	/* Check if there were any checksum errors */
> -	if (unlikely(ipv4 && (csum_bits->ipe || csum_bits->eipe)))
> +	if (unlikely(ipv4 && (csum_bits.ipe || csum_bits.eipe)))
>  		goto checksum_fail;
>  
>  	/* Device could not do any checksum offload for certain extension
>  	 * headers as indicated by setting IPV6EXADD bit
>  	 */
> -	if (unlikely(ipv6 && csum_bits->ipv6exadd))
> +	if (unlikely(ipv6 && csum_bits.ipv6exadd))
>  		return;
>  
>  	/* check for L4 errors and handle packets that were not able to be
>  	 * checksummed due to arrival speed
>  	 */
> -	if (unlikely(csum_bits->l4e))
> +	if (unlikely(csum_bits.l4e))
>  		goto checksum_fail;
>  
> -	if (unlikely(csum_bits->nat && csum_bits->eudpe))
> +	if (unlikely(csum_bits.nat && csum_bits.eudpe))
>  		goto checksum_fail;
>  
>  	/* Handle packets that were not able to be checksummed due to arrival
>  	 * speed, in this case the stack can compute the csum.
>  	 */
> -	if (unlikely(csum_bits->pprs))
> +	if (unlikely(csum_bits.pprs))
>  		return;
>  
>  	/* If there is an outer header present that might contain a checksum
>  	 * we need to bump the checksum level by 1 to reflect the fact that
>  	 * we are indicating we validated the inner checksum.
>  	 */
> -	if (decoded.tunnel_type >= IDPF_RX_PTYPE_TUNNEL_IP_GRENAT)
> +	if (parsed.tunnel_type >= LIBIE_RX_PTYPE_TUNNEL_IP_GRENAT)
>  		skb->csum_level = 1;
>  
> -	/* Only report checksum unnecessary for ICMP, TCP, UDP, or SCTP */
> -	switch (decoded.inner_prot) {
> -	case IDPF_RX_PTYPE_INNER_PROT_ICMP:
> -	case IDPF_RX_PTYPE_INNER_PROT_TCP:
> -	case IDPF_RX_PTYPE_INNER_PROT_UDP:
> -	case IDPF_RX_PTYPE_INNER_PROT_SCTP:
> -		skb->ip_summed = CHECKSUM_UNNECESSARY;
> -		return;
> -	default:
> -		return;
> -	}
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	return;

Is it intentional to change from CHECKSUM_NONE to CHECKSUM_UNNECESSARY
in the default case?

I suppose so, as idpf_rx_csum (the splitq equivalent) does the same
(bar CHECKSUM_COMPLETE depending on descriptor bit).