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.
> 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.
> 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;
>>
