Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)

[Thomas Monjalon <thomas@monjalon.net>]

05/10/2017 22:06, Mokhtar, Amr:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 25/08/2017 15:46, Amr Mokhtar:
> > Did you think about the API required for inline processing (i.e. bbdev combined
> > with ethdev Rx/Tx)?
> 
> The current programming model is that ethdev is being used for input/output and
> the bbdev is offloaded for lookaside acceleration.
> The inline processing topic sounds interesting, this is something we can look at in
> the future. Appreciate if you can share your thoughts in that regard.

When inlining processing in ethdev, it is possible to use rte_flow to
configure processing to be applied on some flows.
However it requires to define the specific action (i.e. processing).
The problem is that you are going to duplicate the API for bbdev processing
if the original API does not fit into a rte_flow action.
The rte_flow actions are still a new idea.
I think it would be nice to have a common structure for
enqueue/dequeue functions and rte_flow configuration.
With this idea, configuring an inline processing would be just
calling an ethdev rte_flow function instead of a bbdev enqueue/dequeue.

Opinions?

[...]
> The way we view this functional split is:
>  - If it is a cryptographic function -> cryptodev
>  - If it is a Wireless L1 function, or in other words, more related to signal processing
>    (coding, scrambling, modulation, ...) -> bbdev
> 
> > With the proposed scheme, if a wireless LTE device implements ZUC algorithm,
> > we will have to support it in a cryptodev PMD while having a bbdev PMD for
> > other wireless functions.
> > Thoughts?
> 
> ZUC stays a cryptographic algorithm, it should go to cryptodev. It's true that ZUC
> is a crypto algorithm used in the mobile wireless domain, but it is not related to signal
> processing (Layer 1).
> bbdev is targeting all or some of those wireless L1 functions.

OK, this definition of the bbdev scope is very interesting.
Please could you explain it in the first lines of bbdev doxygen?

[...]
> > > rte_bbdev_enqueue_ops(dev_id, queue_id, **ops, num_ops)
> > > rte_bbdev_dequeue_ops(dev_id, queue_id, **ops, num_ops)
> > [...]
> > >
> > > **ops is an array of pointers to struct rte_bbdev_op structures which
> > > contain all the details needed by the device to perform a single operation.
> > > As the bbdev interface supports different operation types (although
> > > individual devices may only support a subset of these), it contains a
> > > type field, and a union of parameters for each operation type.
> > >
> > > struct rte_bbdev_op {
> > > 	enum rte_bbdev_op_type type;
> > > 	union {
> > > 		void *generic;
> > > 		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > 		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > 	};
> > > };
> > 
> > I do not understand this part.
> > It seems you want only two generic function to perform processing.
> > I do not see the benefit.
> > It is usually easier to have one function per type of processing.
> > 
> 
> Bbdev devices support Turbo encode and Turbo decode operations.
> Both have separate sets of parameters and different functionalities, but
> each queue is capable of doing either encode or decode (keep in mind
> that this freedom of choice is only applicable before the q gets configured).
> 
> There is only one enqueue function for both enc/dec, and similarly one
> dequeue function for both. The pmd internally interprets its argument
> array of type "rte_bbdev_op" differently based on whether this q was set
> up for enc or dec.
> See this pseudo-code to give more projection of the idea:
> 
> enqueue(struct rte_bbdev_queue_data *q_data, struct rte_bbdev_op *op) {
>     void *queue = q_data->queue_private;
>     struct pmd_prv_queue *q = queue;
> 
>     switch (q->type) {
>     case RTE_BBDEV_OP_TURBO_ENC:
>         struct rte_bbdev_op_turbo_enc *enc = op->turbo_enc;
>         /* do encode */
>         encode(enc);
>         break;
>     case RTE_BBDEV_OP_TURBO_DEC:
>         struct rte_bbdev_op_turbo_dec *dec = op->turbo_edec;
>         /* do decode */
>         decode(dec);
>         break;
>     }
> 
> Since an enqueue was received on a decode queue, then the union is interpreted
> as (rte_bbdev_op_turbo_dec), and vice versa for the encode (rte_bbdev_op_turbo_enc).

I still do not see the benefit.
Why not 4 functions?
	enqueue_enc
	enqueue_dec
	dequeue_enc
	dequeue_dec
Sorry if the question is very basic.