RE: [PATCH v3 1/3] ethdev: enable direct rearm with separate API

["Morten Brørup" <mb@smartsharesystems.com>]

Hi Feifei,

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Monday, 27 February 2023 20.32
> 
> Hi Feifei ,
> 
> 
> > > > +	uint16_t *rearm_start;
> > > > +	uint16_t *rearm_nb;
> > >
> > > I know that for Intel NICs uint16_t is sufficient, wonder would it always
> be
> > > for other vendors?
> > > Another thing to consider the case when ring position wrapping?
> > > Again I know that it is not required for Intel NICs, but would it be
> sufficient
> > > for API that supposed to be general?
> > >
> > For this, we re-define this structure:
> > rte_eth_rxq_rearm_data {
> > 	void *rx_sw_ring;
> > 	uint16_t *rearm_start;
> > 	uint16_t *rearm_nb;
> > }
> > ->
> > struct *rxq_recycle_info {
> > 	rte_mbuf **buf_ring;
> > 	uint16_t *offset = (uint16 *)(&rq->ci);
> > 	uint16_t *end;
> > 	uint16_t ring_size;
> >
> > }
> > For the new structure, *offset is a pointer for rearm-start index of
> > Rx buffer ring (consumer index). *end is a pointer for rearm-end index
> > Of Rx buffer ring (producer index).
> >
> > 1. we look up different pmds,  some pmds using 'uint_16t' as index size like
> intel PMD,
> > some pmds using 'uint32_t' as index size like MLX5 or thunderx PMD.
> > For pmd using 'uint32_t', rearm starts at 'buf_ring[offset & (ring_size -
> 1)]', and 'uint16_t'
> > is enough for ring size.
> 
> Sounds like a smart idea to me.

When configuring an Ethernet device queue, the nb_rx/tx_desc parameter to rte_eth_rx/tx_queue_setup() is uint16_t, so I agree that uint16_t should suffice here too.

I had the following thought, but am not sure. So please take this comment for consideration only:

I think the "& (ring_size -1)" is superfluous, unless a PMD allows its index pointer to exceed the ring size, and performs the same "& (ring_size -1)" when using the index pointer to access its ring.

And if a PMD uses the index pointer like that (i.e. exceeding the ring size), you would need the same wrap protection for a 16 bit index pointer.

> 
> 
> >
> > 2. Good question. In general path, there is a constraint that  'nb_rearm <
> ring_size - rq->ci',
> > This can ensure no ring wrapping in rearm. Thus in direct-rearm, we will
> refer to this to
> > solve ring wrapping.
> 
> Should work, I think...
> Just need not to forget to document it :)

It is this constraint (the guarantee that there is no ring wrapping in a rearm burst) that makes me think that the "& (ring_size -1)" is superfluous.