Message-Id: <25FF2886-3FE3-4B20-9A77-217ADE6502B8@net-swift.com>
Date: Thu, 11 May 2023 16:34:52 +0800
From: "mengyuanlou@...-swift.com" <mengyuanlou@...-swift.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: netdev@...r.kernel.org,
 jiawenwu@...stnetic.com
Subject: Re: [PATCH net-next v4 2/7] net: wangxun: libwx add rx offload
 functions



> 2023年5月10日 19:43，Yunsheng Lin <linyunsheng@...wei.com> 写道：
> 
> On 2023/5/10 17:38, Mengyuan Lou wrote:
> ...
> 
>> +/**
>> + * wx_rx_checksum - indicate in skb if hw indicated a good cksum
>> + * @ring: structure containing ring specific data
>> + * @rx_desc: current Rx descriptor being processed
>> + * @skb: skb currently being received and modified
>> + **/
>> +static void wx_rx_checksum(struct wx_ring *ring,
>> +   union wx_rx_desc *rx_desc,
>> +   struct sk_buff *skb)
>> +{
>> + struct wx_dec_ptype dptype = wx_decode_ptype(WX_RXD_PKTTYPE(rx_desc));
>> +
>> + skb->ip_summed = CHECKSUM_NONE;
>> + skb_checksum_none_assert(skb);
> 
> It does not make much to check skb->ip_summed when it is just
> set one line above.
> 
> Also the "skb->ip_summed = CHECKSUM_NONE" seems unnecessary,
> as alloc/build_skb() all have the below to make sure
> skb->ip_summed is zero:
> 
> memset(skb, 0, offsetofstruct sk_buff, tail))
It’s right.
> 
>> + /* Rx csum disabled */
>> + if (!(ring->netdev->features & NETIF_F_RXCSUM))
>> + return;
>> +
>> + /* if IPv4 header checksum error */
>> + if ((wx_test_staterr(rx_desc, WX_RXD_STAT_IPCS) &&
>> +     wx_test_staterr(rx_desc, WX_RXD_ERR_IPE)) ||
>> +    (wx_test_staterr(rx_desc, WX_RXD_STAT_OUTERIPCS) &&
>> +     wx_test_staterr(rx_desc, WX_RXD_ERR_OUTERIPER))) {
>> + ring->rx_stats.csum_err++;
>> + return;
>> + }
>> +
>> + /* L4 checksum offload flag must set for the below code to work */
>> + if (!wx_test_staterr(rx_desc, WX_RXD_STAT_L4CS))
>> + return;
>> +
>> + /*likely incorrect csum if IPv6 Dest Header found */
> 
> What does "likely incorrect" mean here? If it is incorrect,
> does ring->rx_stats.csum_err need incrementing?

This is a workaround for hardware, which the IPV6EX is on hardware can not
guarantee the correctness of the verification. So just ignored these packages
Check.
> 
>> + if (dptype.prot != WX_DEC_PTYPE_PROT_SCTP && WX_RXD_IPV6EX(rx_desc))
>> + return;
>> +
>> + /* if L4 checksum error */
>> + if (wx_test_staterr(rx_desc, WX_RXD_ERR_TCPE)) {
>> + ring->rx_stats.csum_err++;
>> + 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 (dptype.etype >= WX_DEC_PTYPE_ETYPE_IG) {
>> + skb->csum_level = 1;
>> + skb->encapsulation = 1;
>> + }
>> +
>> + /* It must be a TCP or UDP or SCTP packet with a valid checksum */
>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>> + ring->rx_stats.csum_good_cnt++;
>> +}
>> +
>> +/**
>> + * wx_process_skb_fields - Populate skb header fields from Rx descriptor
>> + * @rx_ring: rx descriptor ring packet is being transacted on
>> + * @rx_desc: pointer to the EOP Rx descriptor
>> + * @skb: pointer to current skb being populated
>> + *
>> + * This function checks the ring, descriptor, and packet information in
>> + * order to populate the hash, checksum, VLAN, timestamp, protocol, and
> 
> For now VLAN, timestamp are not populated yet.
> 
>> + * other fields within the skb.
>> + **/
>> +static void wx_process_skb_fields(struct wx_ring *rx_ring,
>> +  union wx_rx_desc *rx_desc,
>> +  struct sk_buff *skb)
>> +{
>> + wx_rx_hash(rx_ring, rx_desc, skb);
>> + wx_rx_checksum(rx_ring, rx_desc, skb);
>> + skb_record_rx_queue(skb, rx_ring->queue_index);
>> + skb->protocol = eth_type_trans(skb, rx_ring->netdev);
>> +}
>> +
>> /**
>>  * wx_clean_rx_irq - Clean completed descriptors from Rx ring - bounce buf
>>  * @q_vector: structure containing interrupt and ring information
>> @@ -491,8 +586,8 @@ static int wx_clean_rx_irq(struct wx_q_vector *q_vector,
>> /* probably a little skewed due to removing CRC */
>> total_rx_bytes += skb->len;
>> 
>> - skb_record_rx_queue(skb, rx_ring->queue_index);
>> - skb->protocol = eth_type_trans(skb, rx_ring->netdev);
>> + /* populate checksum, timestamp, VLAN, and protocol */
>> + wx_process_skb_fields(rx_ring, rx_desc, skb);
>> napi_gro_receive(&q_vector->napi, skb);
>> 
>> /* update budget accounting */
>> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
>> index 70f5fd168e40..69a9ed7bc2df 100644
>> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
>> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
>> @@ -321,8 +321,31 @@
> 
> ...
> 
>> +
>> +static inline struct wx_dec_ptype wx_decode_ptype(const u8 ptype)
> 
> If the above is only used in one .c file, maybe it does not need
> to be in the .h file?

If I put it to .c file which use it, when compiling the other .c files will say
"warning: ‘wx_ptype_lookup’ defined but not used”.
> 
>> +{
>> + return wx_ptype_lookup[ptype];
>> +}
>> +
>> /* Host Interface Command Structures */
>> struct wx_hic_hdr {
>> u8 cmd;
>> @@ -624,6 +853,11 @@ struct wx_queue_stats {
>> u64 bytes;
>> };
>> 
>> +struct wx_rx_queue_stats {
>> + u64 csum_good_cnt;
>> + u64 csum_err;
>> +};
>> +
>> /* iterator for handling rings in ring container */
>> #define wx_for_each_ring(posm, headm) \
>> for (posm = (headm).ring; posm; posm = posm->next)
>> @@ -665,6 +899,9 @@ struct wx_ring {
>> 
>> struct wx_queue_stats stats;
>> struct u64_stats_sync syncp;
>> + union {
>> + struct wx_rx_queue_stats rx_stats;
>> + };
>> } ____cacheline_internodealigned_in_smp;
>> 
>> struct wx_q_vector {
>> @@ -680,6 +917,7 @@ struct wx_q_vector {
>> struct napi_struct napi;
>> struct rcu_head rcu;    /* to avoid race with update stats on free */
>> 
>> + bool netpoll_rx;
> 
> Unused?
> 
>> char name[IFNAMSIZ + 17];
>> 
>> /* for dynamic allocation of rings associated with this q_vector */
>> 
> 

Powered by blists - more mailing lists