Re: [PATCH net-next v3 1/5] net: wangxun: libwx add tx offload functions

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2023/4/20 18:37, Mengyuan Lou wrote:
> +static u8 wx_get_ipv6_proto(struct sk_buff *skb, int offset)
> +{
> +	struct ipv6hdr *hdr = (struct ipv6hdr *)(skb->data + offset);
> +	u8 nexthdr = hdr->nexthdr;
> +
> +	offset += sizeof(struct ipv6hdr);
> +
> +	while (ipv6_ext_hdr(nexthdr)) {
> +		struct ipv6_opt_hdr _hdr, *hp;
> +
> +		if (nexthdr == NEXTHDR_NONE)
> +			break;
> +
> +		hp = skb_header_pointer(skb, offset, sizeof(_hdr), &_hdr);
> +		if (!hp)
> +			break;

Isn't this a error, which need to be propagated to the caller?

Also, it is a rare case, maybe add a unlikely() for that. Same for other
error handling.

> +
> +		if (nexthdr == NEXTHDR_FRAGMENT)
> +			break;
> +		else if (nexthdr == NEXTHDR_AUTH)
> +			offset +=  ipv6_authlen(hp);
> +		else
> +			offset +=  ipv6_optlen(hp);
> +		nexthdr = hp->nexthdr;
> +	}
> +
> +	return nexthdr;
> +}
> +
>
...

>  static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  				      struct wx_ring *tx_ring)
>  {
>  	u16 count = TXD_USE_COUNT(skb_headlen(skb));
> +	__be16 protocol = skb->protocol;
>  	struct wx_tx_buffer *first;
> +	u8 hdr_len = 0, ptype;
>  	unsigned short f;
> +	u32 tx_flags = 0;
> +	int tso;
>  
>  	/* need: 1 descriptor per page * PAGE_SIZE/WX_MAX_DATA_PER_TXD,
>  	 *       + 1 desc for skb_headlen/WX_MAX_DATA_PER_TXD,
> @@ -864,7 +1327,41 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
>  	first->bytecount = skb->len;
>  	first->gso_segs = 1;
>  
> -	wx_tx_map(tx_ring, first);
> +	/* if we have a HW VLAN tag being added default to the HW one */
> +	if (skb_vlan_tag_present(skb)) {
> +		tx_flags |= skb_vlan_tag_get(skb) << WX_TX_FLAGS_VLAN_SHIFT;
> +		tx_flags |= WX_TX_FLAGS_HW_VLAN;
> +	/* else if it is a SW VLAN check the next protocol and store the tag */
> +	} else if (eth_type_vlan(protocol)) {
> +		struct vlan_hdr *vhdr, _vhdr;
> +
> +		vhdr = skb_header_pointer(skb, ETH_HLEN, sizeof(_vhdr), &_vhdr);
> +		if (!vhdr)
> +			goto out_drop;
> +
> +		protocol = vhdr->h_vlan_encapsulated_proto;

isn't the protocol set here overrided by "protocol = vlan_get_protocol(skb)"
a few line below?

Also, Is "__be16 protocol = skb->protocol" necessary if
"protocol = vlan_get_protocol(skb)" is called unconditionally a few line below
and no one is using it before that?

> +		tx_flags |= ntohs(vhdr->h_vlan_TCI) << WX_TX_FLAGS_VLAN_SHIFT;
> +		tx_flags |= WX_TX_FLAGS_SW_VLAN;

WX_TX_FLAGS_SW_VLAN is set here, but it seems that wx_tx_cmd_type() does not use it?
It that intended? what is it used for?

> +	}
> +	protocol = vlan_get_protocol(skb);
> +
> +	/* record initial flags and protocol */
> +	first->tx_flags = tx_flags;
> +	first->protocol = protocol;
> +

...

> +
> +#define WX_SET_FLAG(_input, _flag, _result) \
> +	(((_flag) <= (_result)) ? \
> +	 ((u32)((_input) & (_flag)) * ((_result) / (_flag))) : \
> +	 ((u32)((_input) & (_flag)) / ((_flag) / (_result))))

Perhaps add a comment for the above macro, it seems a little hard to
understand it.


>  /* iterator for handling rings in ring container */
>  #define wx_for_each_ring(posm, headm) \
>  	for (posm = (headm).ring; posm; posm = posm->next)
> @@ -580,6 +693,9 @@ struct wx_ring {
>  
>  	struct wx_queue_stats stats;
>  	struct u64_stats_sync syncp;
> +	union {
> +		struct wx_tx_queue_stats tx_stats;

It does not seem to be used?

> +	};
>  } ____cacheline_internodealigned_in_smp;
>  
>  struct wx_q_vector {
>