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

[Thomas Monjalon <thomas@monjalon.net>]

07/10/2017 01:27, Mokhtar, Amr:
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday 5 October 2017 23:23
> > To: Mokhtar, Amr <amr.mokhtar@intel.com>
> > Cc: dev@dpdk.org; fbl@redhat.com; aconole@redhat.com; bluca@debian.org
> > Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev)
> > 
> > 05/10/2017 23:55, Mokhtar, Amr:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 03/10/2017 16:29, Mokhtar, Amr:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 25/08/2017 15:46, Amr Mokhtar:
> > > > > > > +int
> > > > > > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues,
> > > > > > > +		const struct rte_bbdev_conf *conf);
> > > > > >
> > > > > > I am not convinced by the "configure all" function in ethdev.
> > > > > > We break the ABI each time we add a new feature to configure.
> > > > > > And it does not really help to have all configurations in one struct.
> > > > > > Would you mind to split the struct rte_bbdev_conf and split the
> > > > > > function accordingly?
> > > > >
> > > > > There is nothing to split tbh. The only parameter it has is the socket_id.
> > > > > And in fact, it's optional, can be null. The only config we need is
> > num_queues.
> > > >
> > > > Indeed, there is nothing in this struct.
> > > > If you need only to allocate queues, you just have to rename this function.
> > > >
> > > > > I don't see in the near future that we may need to add more config params.
> > > > > As a side, in the time of the implementation we were trying to
> > > > > avoid any diversions from the current design ideology of ethdev and
> > cryptodev.
> > > >
> > > > There is no ideology in ethdev, just some mistakes ;)
> > > >
> > > > > Can we leave it for consideration with future releases?
> > > >
> > > > No it should be addressed from the beginning.
> > > >
> > > > When you will need to add something more to configure port-wise, you
> > > > should add a new function instead of breaking the ABI of the global conf
> > struct.
> > > > That's why the configure option should be more specialized.
> > > >
> > > > Distro people were complaining about ABI breakage last week.
> > > > This is exactly an example of how to avoid it from the beginning.
> > > >
> > >
> > > Ok, got your point. I was looking at it from an API-only standpoint.
> > > How about modifying it into?
> > > int
> > > rte_bbdev_setup_queues(uint16_t dev_id, uint16_t num_queues, int
> > > socket_id);
> > 
> > Yes OK
> > 
> > [...]
> > > > > > > +struct __rte_cache_aligned rte_bbdev {
> > > > > > > +	rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue
> > function */
> > > > > > > +	rte_bbdev_dequeue_ops_t dequeue_ops;  /**< Dequeue
> > function */
> > > > > > > +	const struct rte_bbdev_ops *dev_ops;  /**< Functions
> > > > > > > +exported by PMD
> > > > > > */
> > > > > > > +	struct rte_bbdev_data *data;  /**< Pointer to device data */
> > > > > > > +	bool attached;  /**< If device is currently attached or not
> > > > > > > +*/
> > > > > >
> > > > > > What "attached" means?
> > > > > > I'm afraid you are trying to manage hotplug in the wrong layer.
> > > > >
> > > > > Hotplug is not supported in the current release.
> > > >
> > > > It is not answering the question.
> > > > What is an "attached" device?
> > >
> > > "Attached" means that the PCI device was probed and the bbdev device slot is
> > allocated.
> > > For software devices, means that a virtual bbdev device (vdev) is allocated for
> > bbdev.
> > > Same way the "attached" approach used in cryptodev.
> > 
> > Not sure to understand.
> > If "attached" means "allocated", when is it false?
> 
> Currently in bbdev, it is set to true and never goes false.
> As I said the Hotplug feature is not fully supported in the current version. I can remove that flag for now.
> 
> But generally, it should be cleared to false when rte_pci_driver->remove function is called. (Hotplug?)

Hotplug is still a work in progress in DPDK.
Please remove this flag if it is useless.
We will add something if needed when hotplug support will be better designed.

> > [...]
> > > > > > > +/** Structure specifying a single operation */ struct rte_bbdev_op {
> > > > > > > +	enum rte_bbdev_op_type type;  /**< Type of this operation */
> > > > > > > +	int status;  /**< Status of operation that was performed */
> > > > > > > +	struct rte_mempool *mempool;  /**< Mempool which op
> > instance
> > > > > > > +is in
> > > > > > */
> > > > > > > +	void *opaque_data;  /**< Opaque pointer for user data */
> > > > > > > +	/**
> > > > > > > +	 * Anonymous union of operation-type specific parameters.
> > > > > > > +When
> > > > > > allocated
> > > > > > > +	 * using rte_bbdev_op_pool_create(), space is allocated for the
> > > > > > > +	 * parameters at the end of each rte_bbdev_op structure, and
> > the
> > > > > > > +	 * pointers here point to it.
> > > > > > > +	 */
> > > > > > > +	RTE_STD_C11
> > > > > > > +	union {
> > > > > > > +		void *generic;
> > > > > > > +		struct rte_bbdev_op_turbo_dec *turbo_dec;
> > > > > > > +		struct rte_bbdev_op_turbo_enc *turbo_enc;
> > > > > > > +	};
> > > > > > > +};
> > > > > >
> > > > > > I am not sure it is a good idea to fit every operations in the
> > > > > > same struct and the same functions.
> > > > >
> > > > > Due to the fact that our design adopts this idea that a device can
> > > > > support both the encode and decode operations.
> > > > > Then, at the time of PMD registration, the enqueue functions is allocated.
> > > > > This enqueue() function is common for both operations.
> > > > > This fitted operation structure is essential for the driver to
> > > > > decide on the
> > > > operation.
> > > >
> > > > Sorry I do not understand why you must have a "generic operation".
> > > > Please, could you try again to explain this design to someone not
> > > > fully understanding how turbo enc/dec works?
> > >
> > > Oh, sorry, I was not paying attention that you're referring to "void *generic"
> > > It is just a place-holder for any other operation types. Can be removed if you
> > like.
> > 
> > No I was not referring to void *generic.
> > It is the same question as in the RFC.
> > I don't understand the benefit of grouping different things in an union.
> 
> There is no benefit, this is a restriction because there is only one function pointer
> for enq and another one for deq in the ops structure. Again for the same reason
> of trying to keep things in sync with ethdev and cryptodev.
> I've always wanted to make it as you proposed, that way it is more performant
> (no checking for the type of operation.) If this is agreed, I will do it with all my pleasure :)
> 
> The optimum solution though IMHO would be to make the generic enq/deq function pointers
> per queue, instead of being per device; that way every enqueue goes straight to
> the queue-specific function that matches its operation type.
> Notice that currently we have turbo_enc/turbo_dec, but in the future we may have more..

Please do not impose some restrictions to your API just because you want
to mimic ethdev.
Feel free to innovate :)