lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
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