Re: [RFC 2/5] ethdev: introduce the affinity field in Tx queue API

[Thomas Monjalon <thomas@monjalon.net>]

18/01/2023 15:44, Jiawei(Jonny) Wang:
> > 21/12/2022 11:29, Jiawei Wang:
> > > For the multiple hardware ports connect to a single DPDK port
> > > (mhpsdp), the previous patch introduces the new rte flow item to match
> > > the port affinity of the received packets.
> > >
> > > This patch adds the tx_affinity setting in Tx queue API, the affinity
> > > value reflects packets be sent to which hardware port.
> > 
> > I think "affinity" means we would like packet to be sent on a specific hardware
> > port, but it is not mandatory.
> > Is it the meaning you want? Or should it be a mandatory port?
> 
> Right, it's optional setting not mandatory.

I think there is a misunderstanding.
I mean that "affinity" with port 0 may suggest that we try
to send to port 0 but sometimes the packet will be sent to port 1.

And I think you want the packet to be always sent to port 0
if affinity is 0, right?

If yes, I think the word "affinity" does not convey the right idea.
And again, the naming should give the idea that we are talking about
multiple ports merged in one DPDK port.

> > > Adds the new tx_affinity field into the padding hole of rte_eth_txconf
> > > structure, the size of rte_eth_txconf keeps the same. Adds a suppress
> > > type for structure change in the ABI check file.
> > >
> > > This patch adds the testpmd command line:
> > > testpmd> port config (port_id) txq (queue_id) affinity (value)
> > >
> > > For example, there're two hardware ports connects to a single DPDK
> > 
> > connects -> connected
> 
> OK, will fix in next version.
> 
> > > port (port id 0), and affinity 1 stood for hard port 1 and affinity
> > > 2 stood for hardware port 2, used the below command to config tx
> > > affinity for each TxQ:
> > > 	port config 0 txq 0 affinity 1
> > > 	port config 0 txq 1 affinity 1
> > > 	port config 0 txq 2 affinity 2
> > > 	port config 0 txq 3 affinity 2
> > >
> > > These commands config the TxQ index 0 and TxQ index 1 with affinity 1,
> > > uses TxQ 0 or TxQ 1 send packets, these packets will be sent from the
> > > hardware port 1, and similar with hardware port 2 if sending packets
> > > with TxQ 2 or TxQ 3.
> > 
> > [...]
> > > @@ -212,6 +212,10 @@ API Changes
> > > +* ethdev: added a new field:
> > > +
> > > +  - Tx affinity per-queue ``rte_eth_txconf.tx_affinity``
> > 
> > Adding a new field is not an API change because existing applications don't
> > need to update their code if they don't care this new field.
> > I think you can remove this note.
> 
> OK, will remove in next version.
> 
> > > --- a/lib/ethdev/rte_ethdev.h
> > > +++ b/lib/ethdev/rte_ethdev.h
> > > @@ -1138,6 +1138,7 @@ struct rte_eth_txconf {
> > >  				      less free descriptors than this value. */
> > >
> > >  	uint8_t tx_deferred_start; /**< Do not start queue with
> > > rte_eth_dev_start(). */
> > > +	uint8_t tx_affinity; /**< Drives the setting of affinity per-queue.
> > > +*/
> > 
> > Why "Drives"? It is the setting, right?
> > rte_eth_txconf is per-queue so no need to repeat.
> > I think a good comment here would be to mention it is a physical port index for
> > mhpsdp.
> > Another good comment would be to specify how ports are numbered.
> 
> OK, will update the comment for this new setting.
> 
> Thanks.