DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
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)
Date: Fri, 06 Oct 2017 00:22:58 +0200	[thread overview]
Message-ID: <34263409.OrRmme2Ibo@xps> (raw)
In-Reply-To: <3D3765A8CDB52A4C8B410430AA19CB236EC35318@IRSMSX104.ger.corp.intel.com>

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?

[...]
> > > > > +/** 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.

  reply	other threads:[~2017-10-05 22:23 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-25 13:46 Amr Mokhtar
2017-08-25 13:46 ` Amr Mokhtar
2017-09-01 19:38   ` Mokhtar, Amr
2017-09-21 14:34   ` Thomas Monjalon
2017-10-05 20:06     ` Mokhtar, Amr
2017-10-05 20:49       ` Thomas Monjalon
2017-09-01 20:03 ` Stephen Hemminger
2017-09-01 21:35   ` Mokhtar, Amr
2017-09-21 14:56 ` Thomas Monjalon
2017-10-03 14:29   ` Mokhtar, Amr
2017-10-03 15:17     ` Thomas Monjalon
2017-10-04 17:11       ` Flavio Leitner
2017-10-05 21:55       ` Mokhtar, Amr
2017-10-05 22:22         ` Thomas Monjalon [this message]
2017-10-06 23:27           ` Mokhtar, Amr
2017-10-07 11:42             ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=34263409.OrRmme2Ibo@xps \
    --to=thomas@monjalon.net \
    --cc=aconole@redhat.com \
    --cc=amr.mokhtar@intel.com \
    --cc=bluca@debian.org \
    --cc=dev@dpdk.org \
    --cc=fbl@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).