Re: [dpdk-dev] [PATCH 01/13] ethdev: support setup function for hairpin queue

[Ori Kam <orika@mellanox.com>]

Hi Andrew,

@Thomas Monjalon, @Ferruh Yigit

Please comment if you have any issues with my answer.

Thanks,
Ori

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Thursday, October 3, 2019 4:26 PM
> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; jingjing.wu@intel.com; stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH 01/13] ethdev: support setup function for
> hairpin queue
> 
> Hi Ori,
> 
> @Thomas, @Ferruh, please, see question below.
> 
> On 10/2/19 3:19 PM, Ori Kam wrote:
> > Hi Andrew,
> >
> > Sorry it took me some time to responded, (I'm on vacation 😊)
> > I think we are in most cases in agreement. The only open issue is the
> > checks so please see my comments below.
> > As soon as we get to understanding about this issue, I will start working on V2.
> >
> > Thanks,
> > Ori
> 
> [snip]
> 
> >>>>>>> @@ -1769,6 +1793,60 @@ int rte_eth_rx_queue_setup(uint16_t
> port_id,
> >>>>>> uint16_t rx_queue_id,
> >>>>>>>      		struct rte_mempool *mb_pool);
> >>>>>>>
> >>>>>>>      /**
> >>>>>>> + * @warning
> >>>>>>> + * @b EXPERIMENTAL: this API may change, or be removed, without
> prior
> >>>>>>> + notice
> >>>>>>> + *
> >>>>>>> + * Allocate and set up a hairpin receive queue for an Ethernet device.
> >>>>>>> + *
> >>>>>>> + * The function set up the selected queue to be used in hairpin.
> >>>>>>> + *
> >>>>>>> + * @param port_id
> >>>>>>> + *   The port identifier of the Ethernet device.
> >>>>>>> + * @param rx_queue_id
> >>>>>>> + *   The index of the receive queue to set up.
> >>>>>>> + *   The value must be in the range [0, nb_rx_queue - 1] previously
> supplied
> >>>>>>> + *   to rte_eth_dev_configure().
> >>>>>> Is any Rx queue may be setup as hairpin queue?
> >>>>>> Can it be still used for regular traffic?
> >>>>>>
> >>>>> No if a queue is used as hairpin it can't be used for normal traffic.
> >>>>> This is also why I like the idea of two different functions, in order to
> create
> >>>>> This distinction.
> >>>> If so, do we need at least debug-level checks in Tx/Rx burst functions?
> >>>> Is it required to patch rte flow RSS action to ensure that Rx queues of
> >>>> only one kind are specified?
> >>>> What about attempt to add Rx/Tx callbacks for hairpin queues?
> >>>>
> >>> I think the checks should be done in PMD level. Since from high level they
> are the
> >>> same.
> >> Sorry, I don't understand why. If something could be checked on generic
> level,
> >> it should be done to avoid duplication in all drivers.
> > The issue with this approach is that at the ethdev level we don't know
> anything about the queue.
> > This will mean that we will need to add extra functions to query the queue
> type for each PMD.
> > We could also assume that if to get type function exist in the pmd then the
> queue is always standard queue.
> > So my suggestion if you would like to move the checks is to add queue type
> enum in the ethdev level, and add
> > function call to query the queue type. What do you think?
> 
> I would consider to use dev_data rx_queue_state and tx_queue_state to
> keep the information to have it directly available without extra function
> calls. Or add extra information. dev_data is internal and it looks like not
> a problem. What do you think?
> 

I like the new state idea, it will save some memory in the dev_data, compared to having it
in the dev_data. Will also avoid extra ABI change.

> >>> Call backs for Rx/Tx doesn't make sense, since the idea is to bypass the
> CPU.
> >> If so, I think rte_eth_add_tx_callback() should be patched to return an
> >> error
> >> if specified queue is hairpin. Same for Rx.
> >> Any other cases?
> >>
> > Same answer as above.
> 
> [snip]
> 
> Andrew.