[prev in list] [next in list] [prev in thread] [next in thread] 

List:       intel-wired-lan
Subject:    Re: [Intel-wired-lan] [PATCH RFC net-next 01/34] idpf: reuse libie's definitions of parsed ptype str
From:       Willem de Bruijn <willemdebruijn.kernel () gmail ! com>
Date:       2023-12-27 15:43:34
Message-ID: 658c46269aa52_a33e629442 () willemb ! c ! googlers ! com ! notmuch
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]