Re: [PATCH RFC net-next 01/34] idpf: reuse libie's definitions of parsed ptype structures

[Willem de Bruijn]

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@xxxxxxxxx>
> ---
>  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).