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