Re: [PATCH RESEND bpf-next 02/15] ice: make RX HW timestamp reading code more reusable

[Larysa Zaremba]

On Fri, May 19, 2023 at 06:52:13PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> Date: Fri, 12 May 2023 17:25:54 +0200
> 
> > Previously, we only needed RX HW timestamp in skb path,
> > hence all related code was written with skb in mind.
> > But with the addition of XDP hints via kfuncs to the ice driver,
> > the same logic will be needed in .xmo_() callbacks.
> 
> [...]
> 
> > @@ -2176,9 +2174,8 @@ ice_ptp_rx_hwtstamp(struct ice_rx_ring *rx_ring,
> >  	ts_high = le32_to_cpu(rx_desc->wb.flex_ts.ts_high);
> >  	ts_ns = ice_ptp_extend_32b_ts(cached_time, ts_high);
> >  
> > -	hwtstamps = skb_hwtstamps(skb);
> > -	memset(hwtstamps, 0, sizeof(*hwtstamps));
> > -	hwtstamps->hwtstamp = ns_to_ktime(ts_ns);
> > +	*dst = ts_ns;
> > +	return true;
> 
> Can't we use the same I wrote in the prev. comment, i.e. return 0 or
> timestamp? I don't think ts == 0 is valid.
>

Agreed with this in the answer to the previous email :)
 
> >  }
> >  
> >  /**
> 
> [...]
> 
> > + * The driver receives a notification in the receive descriptor with timestamp.
> > + * The timestamp is in ns, so we must convert the result first.
> > + */
> > +static void
> > +ice_ptp_rx_hwts_to_skb(struct ice_rx_ring *rx_ring,
> > +		       union ice_32b_rx_flex_desc *rx_desc,
> > +		       struct sk_buff *skb)
> > +{
> > +	struct skb_shared_hwtstamps *hwtstamps;
> > +	u64 ts_ns;
> > +
> > +	if (!ice_ptp_copy_rx_hwts_from_desc(rx_ring, rx_desc, &ts_ns))
> > +		return;
> > +
> > +	hwtstamps = skb_hwtstamps(skb);
> > +	memset(hwtstamps, 0, sizeof(*hwtstamps));
> > +	hwtstamps->hwtstamp = ns_to_ktime(ts_ns);
> 
> Ok, my optimizations aren't in this series :D
> If you look at the hwtimestamps in skb, you'll see all that can be
> minimized to just:
> 
> 	*skb_hwtstamps(skb) = (struct skb_shared_hwtstamps){
> 		.hwtstamp	= ns_to_ktime(ts_ns),
> 	};
> 
> Compiler will probably do its job, but I wouldn't always rely on it.
> Sometimes it's even able to not expand memset(8 bytes) to *(u64 *) = 0.

Ok, will fix.

> 
> > +}
> > +
> >  /**
> >   * ice_process_skb_fields - Populate skb header fields from Rx descriptor
> >   * @rx_ring: Rx descriptor ring packet is being transacted on
> > @@ -210,7 +235,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
> >  	ice_rx_csum(rx_ring, skb, rx_desc, ptype);
> >  
> >  	if (rx_ring->ptp_rx)
> > -		ice_ptp_rx_hwtstamp(rx_ring, rx_desc, skb);
> > +		ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb);
> >  }
> >  
> >  /**
> 
> Thanks,
> Olek