07/10/2017 01:27, Mokhtar, Amr: > > > -----Original Message----- > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > Sent: Thursday 5 October 2017 23:23 > > To: Mokhtar, Amr <amr.mokh...@intel.com> > > Cc: dev@dpdk.org; f...@redhat.com; acon...@redhat.com; bl...@debian.org > > Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev) > > > > 05/10/2017 23:55, Mokhtar, Amr: > > > From: Thomas Monjalon [mailto:tho...@monjalon.net] > > > > 03/10/2017 16:29, Mokhtar, Amr: > > > > > From: Thomas Monjalon [mailto:tho...@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 :)