Re: [PATCH net-next v2 2/3] net: atlantic: Implement xdp data plane

[Taehee Yoo <ap420073@gmail.com>]

On 3/21/22 23:18, Igor Russkikh wrote:
 > Hi Taehee,
 >

Hi Igor,

Thank you so much for your review!

 > Thanks for taking care of that!
 > Just for your information - I've started xdp draft sometime ago,
 > but never had a time to complete it.
 > If interested, you can check it here:
 > 
https://github.com/Aquantia/AQtion/commit/165cc46cb3fa68eca3110d846db1744a0feee916
 >

Thanks a lot for this information :)

 > Couple of comments on your implementation follows.
 >
 > On 3/19/2022 3:04 PM, Taehee Yoo wrote:
 >> It supports XDP_PASS, XDP_DROP and multi buffer.
 >>
 >>  From now on aq_nic_map_skb() supports xdp_frame to send packet.
 >> So, TX path of both skb and xdp_frame can use aq_nic_map_skb().
 >> aq_nic_xmit() is used to send packet with skb and internally it
 >> calls aq_nic_map_skb(). aq_nic_xmit_xdpf() is used to send packet with
 >> xdp_frame and internally it calls aq_nic_map_skb().
 >
 >>   unsigned int aq_nic_map_skb(struct aq_nic_s *self, struct sk_buff 
*skb,
 >> -			    struct aq_ring_s *ring)
 >> +			    struct xdp_frame *xdpf, struct aq_ring_s
 >> *ring)
 >>   {
 >
 > Its not a huge problem, but here you are using one function 
(aq_nic_map_skb) with two
 > completely separate paths: either skb != NULL or xdpf != NULL.
 > This makes the function abit cumbersome and error prone.
 >
 >> +	if (xdpf) {
 >> +		sinfo = xdp_get_shared_info_from_frame(xdpf);
 >> +		total_len = xdpf->len;
 >> +		dx_buff->len = total_len;
 >> +		data_ptr = xdpf->data;
 >> +		if (xdp_frame_has_frags(xdpf)) {
 >> +			nr_frags = sinfo->nr_frags;
 >> +			total_len += sinfo->xdp_frags_size;
 >> +		}
 >> +		goto start_xdp;
 >
 > May be instead of doing this jump - just introduce a separate function
 > like `aq_map_xdp` specially for xdp case.
 >

I agree with it, I will separate it.

 >> +int aq_ring_rx_clean(struct aq_ring_s *self,
 >> +		     struct napi_struct *napi,
 >> +		     int *work_done,
 >> +		     int budget)
 >> +{
 >> +	if (static_branch_unlikely(&aq_xdp_locking_key))
 >> +		return __aq_ring_xdp_clean(self, napi, work_done, budget);
 >> +	else
 >> +		return __aq_ring_rx_clean(self, napi, work_done, budget);
 >> +}
 >
 > Is that really required to split into `xdp_clean` and `rx_clean` ?
 > They are very similar, may be try to unify?
 >

Yes, these two functions are similar.
But there is 2 reason.
1. flip strategy issue.
In order to use flip strategy, page reference count is 
used(page_ref_{inc | dec}).
The flip strategy can not be used when rx frame size is over 2K but 3K 
rx frame size is used if XDP is enabled.
So, if XDP is enabled, the flip strategy is always impossible therefore 
page_ref_inc() is unnecessary and expensive.
__aq_ring_xdp_clean() doesn't call page_ref_inc().

2. page_offset and page_order issue
When xdp is enabled, 0-order must be used and over 256 page_offset 
should be used.
But xdp is not enabled, multi-order can be used and 0 page_offset is used.
Because of different required values, I made separated functions.
Unifying them is I think possible, but logic would be more complex.

 > Regards,
 >    Igor

Thank you so much!

Taehee