Re: [Intel-wired-lan] [PATCH iwl-next v6 2/2] ice: add E830 Earliest TxTime First Offload support

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Aug 13, 2025 at 11:32:10AM -0700, Greenwalt, Paul wrote:
> 
> 
> On 8/12/2025 8:15 AM, Maciej Fijalkowski wrote:
> > On Mon, Aug 11, 2025 at 04:44:06AM -0400, Paul Greenwalt wrote:
> > 
> >>  
> >> +/**
> >> + * ice_cfg_txtime - configure Tx Time for the Tx ring
> >> + * @tx_ring: pointer to the Tx ring structure
> >> + *
> >> + * Return: 0 on success, negative value on failure.
> >> + */
> >> +static int ice_cfg_txtime(struct ice_tx_ring *tx_ring)
> >> +{
> >> +	int err, timeout = 50;
> >> +	struct ice_vsi *vsi;
> >> +	struct device *dev;
> >> +	struct ice_pf *pf;
> >> +	u32 queue;
> >> +
> >> +	if (!tx_ring)
> >> +		return -EINVAL;
> >> +
> >> +	vsi = tx_ring->vsi;
> >> +	pf = vsi->back;
> >> +	while (test_and_set_bit(ICE_CFG_BUSY, pf->state)) {
> >> +		timeout--;
> >> +		if (!timeout)
> >> +			return -EBUSY;
> >> +		usleep_range(1000, 2000);
> >> +	}
> >> +
> >> +	queue = tx_ring->q_index;
> >> +	dev = ice_pf_to_dev(pf);
> >> +	err = ice_qp_dis(vsi, queue);
> >> +	if (err) {
> >> +		dev_err(dev, "Failed to disable Tx queue %d for TxTime configuration\n",
> >> +			queue);
> >> +		goto exit;
> > 
> > nit: in this case you leave queue pair in limbo state. i would be trying
> > to bring it up regardless whether disable succeeded.
> > 
> 
> I will make this change.
> 
> > 
> > (...)
> > 
> >>  
> >>  dma_error:
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> >> index 2fd8e78178a2..be74851eadd4 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> >> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> >> @@ -310,6 +310,12 @@ enum ice_dynamic_itr {
> >>  #define ICE_TX_LEGACY	1
> >>  
> >>  /* descriptor ring, associated with a VSI */
> >> +struct ice_tstamp_ring {
> >> +	struct ice_tx_ring *tx_ring;	/* Backreference to associated Tx ring */
> >> +	dma_addr_t dma;			/* physical address of ring */
> >> +	struct rcu_head rcu;            /* to avoid race on free */
> >> +} ____cacheline_internodealigned_in_smp;
> >> +
> >>  struct ice_rx_ring {
> >>  	/* CL1 - 1st cacheline starts here */
> >>  	void *desc;			/* Descriptor ring memory */
> >> @@ -387,11 +393,22 @@ struct ice_tx_ring {
> >>  	struct xsk_buff_pool *xsk_pool;
> >>  	u16 next_to_use;
> >>  	u16 next_to_clean;
> >> +	u16 tstamp_next_to_use;		/* Time stamp ring next to use */
> >> +	u16 tstamp_count;		/* Time stamp ring descriptors count */
> >> +	u8 __iomem *tstamp_tail;	/* Time stamp ring tail pointer */
> >> +	void *tstamp_desc;		/* Time stamp descriptor ring memory */
> > 
> > we spoke about putting these onto ice_tstamp_ring...if i am reading this
> > right you want to avoid touching ice_tstamp_ring in hot-path - is that
> > correct?
> > 
> 
> The time stamp ring next_to_use, count and *desc can be moved into the
> struct ice_tstamp_ring. The reasoning for place them in the ice_tx_ring
> 2nd cacheline was to avoid possible cache misses.

Paul, from my perspective scoping everything in ice_tstamp_ring and
putting pointer to it on hot cacheline would be easier to follow,
code-wise. However, if you might be having any data that would back your
concerns or if you still insist on keeping the struct layout you're
proposing here then let us have a comment describing the reason behind
this design?

> 
> > if that ring is etf-enabled then does it ever process the normal tx
> > traffic? what i'm trying to ask is whether you considered putting these
> > members onto union with next_to_use, count and *desc.
> > 
> 
> The E830 ETF support requires the use of the Tx ring (i.e. next_to_use,
> count, and *desc) as well as the need for a new time stamp ring. The
> time stamp ring contains a the time stamp and a reference to the Tx ring
> descriptor, so both rings are used when etf-enabled.

ok, sorry for brain fart on my side here.

> 
> > how different is layout of ice_tx_ring after your change?
> > 
> 
> The variables that where moved out of the ice_tx_ring 2nd cacheline are
> not accessed in the fast path.
> 
> Thanks,
> Paul
> 
> > rest of the code looks good to me now, however i still would like to
> > clarify things mentioned above.
> > 
> >>  	u16 q_handle;			/* Queue handle per TC */
> >>  	u16 reg_idx;			/* HW register index of the ring */
> >>  	u16 count;			/* Number of descriptors */
> >>  	u16 q_index;			/* Queue number of ring */
> >>  	u16 xdp_tx_active;
> >> +	u16 quanta_prof_id;
> >> +	u8 dcb_tc;			/* Traffic class of ring */
> >> +#define ICE_TX_FLAGS_RING_XDP		BIT(0)
> >> +#define ICE_TX_FLAGS_RING_VLAN_L2TAG1	BIT(1)
> >> +#define ICE_TX_FLAGS_RING_VLAN_L2TAG2	BIT(2)
> >> +#define ICE_TX_FLAGS_TXTIME		BIT(3)
> >> +	u8 flags;
> >>  	/* stats structs */
> >>  	struct ice_ring_stats *ring_stats;
> >>  	/* CL3 - 3rd cacheline starts here */
> >> @@ -401,13 +418,7 @@ struct ice_tx_ring {
> >>  	struct ice_ptp_tx *tx_tstamps;
> >>  	spinlock_t tx_lock;
> >>  	u32 txq_teid;			/* Added Tx queue TEID */
> >> -	/* CL4 - 4th cacheline starts here */
> >> -#define ICE_TX_FLAGS_RING_XDP		BIT(0)
> >> -#define ICE_TX_FLAGS_RING_VLAN_L2TAG1	BIT(1)
> >> -#define ICE_TX_FLAGS_RING_VLAN_L2TAG2	BIT(2)
> >> -	u8 flags;
> >> -	u8 dcb_tc;			/* Traffic class of ring */
> >> -	u16 quanta_prof_id;
> >> +	struct ice_tstamp_ring *tstamp_ring;
> >>  } ____cacheline_internodealigned_in_smp;
> >>  
>