[dpdk-dev,RFC,v2] libeventdev: event driven programming model framework for DPDK

  On Fri, Oct 28, 2016 at 02:36:48PM +0530, Jerin Jacob wrote:
> On Fri, Oct 28, 2016 at 09:36:46AM +0100, Bruce Richardson wrote:
> > On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> > > On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > > > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > > > On Wed, Oct 26, 2016 at 12:11:03PM +0000, Van Haaren, Harry wrote:
> > > > > > > -----Original Message-----
> > > > rte_event_queue_conf, with possible values:
> > > > * atomic
> > > > * ordered
> > > > * parallel
> > > > * mixed - allowing all 3 types. I think allowing 2 of three types might
> > > >     make things too complicated.
> > > > 
> > > > An open question would then be how to behave when the queue type and
> > > > requested event type conflict. We can either throw an error, or just
> > > > ignore the event type and always treat enqueued events as being of the
> > > > queue type. I prefer the latter, because it's faster not having to
> > > > error-check, and it pushes the responsibility on the app to know what
> > > > it's doing.
> > > 
> > > How about making default as "mixed" and let application configures what
> > > is not required?. That way application responsibility is clear.
> > > something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> > > with default.
> > > 
> > I suppose it could work, but why bother doing that? If an app knows it's
> > only going to use one traffic type, why not let it just state what it
> > will do rather than try to specify what it won't do. If mixed is needed,
> 
> My thought was more inline with ethdev spec, like, ref-count is default,
> if application need exception then set ETH_TXQ_FLAGS_NOREFCOUNT. But it is OK, if
> you need other way.
> 
> > then it's easy enough to specify - and we can make it the zero/default
> > value too.
> 
> OK. Then we will make MIX as zero/default and add "allowed_event_types" in
> event queue config.
>

Bruce,

I have tried to make it as "allowed_event_types" in event queue config.
However, rte_event_queue_default_conf_get() can also take NULL for default
configuration. So I think, It makes sense to go with negation approach
like ethdev to define the default to avoid confusion on the default. So
I am thinking like below now,

➜ [master][libeventdev] $ git diff
 struct rte_event_queue_conf {

Thoughts?

 
> /Jerin
> 
> > 
> > Our software implementation for now, only supports one type per queue -
> > which we suspect should meet a lot of use-cases. We'll have to see about
> > adding in mixed types in future.
> > 
> > /Bruce
  

On Wed, Nov 02, 2016 at 04:55:22PM +0530, Jerin Jacob wrote:
> On Fri, Oct 28, 2016 at 02:36:48PM +0530, Jerin Jacob wrote:
> > On Fri, Oct 28, 2016 at 09:36:46AM +0100, Bruce Richardson wrote:
> > > On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> > > > On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > > > > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > > > > On Wed, Oct 26, 2016 at 12:11:03PM +0000, Van Haaren, Harry wrote:
> > > > > > > > -----Original Message-----
> > > > > rte_event_queue_conf, with possible values:
> > > > > * atomic
> > > > > * ordered
> > > > > * parallel
> > > > > * mixed - allowing all 3 types. I think allowing 2 of three types might
> > > > >     make things too complicated.
> > > > > 
> > > > > An open question would then be how to behave when the queue type and
> > > > > requested event type conflict. We can either throw an error, or just
> > > > > ignore the event type and always treat enqueued events as being of the
> > > > > queue type. I prefer the latter, because it's faster not having to
> > > > > error-check, and it pushes the responsibility on the app to know what
> > > > > it's doing.
> > > > 
> > > > How about making default as "mixed" and let application configures what
> > > > is not required?. That way application responsibility is clear.
> > > > something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> > > > with default.
> > > > 
> > > I suppose it could work, but why bother doing that? If an app knows it's
> > > only going to use one traffic type, why not let it just state what it
> > > will do rather than try to specify what it won't do. If mixed is needed,
> > 
> > My thought was more inline with ethdev spec, like, ref-count is default,
> > if application need exception then set ETH_TXQ_FLAGS_NOREFCOUNT. But it is OK, if
> > you need other way.
> > 
> > > then it's easy enough to specify - and we can make it the zero/default
> > > value too.
> > 
> > OK. Then we will make MIX as zero/default and add "allowed_event_types" in
> > event queue config.
> >
> 
> Bruce,
> 
> I have tried to make it as "allowed_event_types" in event queue config.
> However, rte_event_queue_default_conf_get() can also take NULL for default
> configuration. So I think, It makes sense to go with negation approach
> like ethdev to define the default to avoid confusion on the default. So
> I am thinking like below now,
> 
> ➜ [master][libeventdev] $ git diff
> diff --git a/rte_eventdev.h b/rte_eventdev.h
> index cf22b0e..cac4642 100644
> --- a/rte_eventdev.h
> +++ b/rte_eventdev.h
> @@ -429,6 +429,12 @@ rte_event_dev_configure(uint8_t dev_id, struct
> rte_event_dev_config *config);
>   *
>   *  \see rte_event_port_setup(), rte_event_port_link()
>   */
> +#define RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE      (1ULL << 1)
> +/**< Skip configuring atomic schedule type resources */
> +#define RTE_EVENT_QUEUE_CFG_NOORDERED_TYPE     (1ULL << 2)
> +/**< Skip configuring ordered schedule type resources */
> +#define RTE_EVENT_QUEUE_CFG_NOPARALLEL_TYPE    (1ULL << 3)
> +/**< Skip configuring parallel schedule type resources */
> 
>  /** Event queue configuration structure */
>  struct rte_event_queue_conf {
> 
> Thoughts?
> 

I'm ok with the default as being all types, in the case where NULL is
specified for the parameter. It does make the most sense.

However, for the cases where the user does specify what they want, I
think it does make more sense, and is easier on the user for things to
be specified in a positive, rather than negative sense. For a user who
wants to just use atomic events, having to specify that as "not-reordered
and not-unordered" just isn't as clear! :-)

/Bruce
  

On Wed, Nov 02, 2016 at 11:35:51AM +0000, Bruce Richardson wrote:
> On Wed, Nov 02, 2016 at 04:55:22PM +0530, Jerin Jacob wrote:
> > On Fri, Oct 28, 2016 at 02:36:48PM +0530, Jerin Jacob wrote:
> > > On Fri, Oct 28, 2016 at 09:36:46AM +0100, Bruce Richardson wrote:
> > > > On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> > > > > On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > > > > > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > > > How about making default as "mixed" and let application configures what
> > > > > is not required?. That way application responsibility is clear.
> > > > > something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> > > > > with default.
> > > > > 
> > > > I suppose it could work, but why bother doing that? If an app knows it's
> > > > only going to use one traffic type, why not let it just state what it
> > > > will do rather than try to specify what it won't do. If mixed is needed,
> > > 
> > > My thought was more inline with ethdev spec, like, ref-count is default,
> > > if application need exception then set ETH_TXQ_FLAGS_NOREFCOUNT. But it is OK, if
> > > you need other way.
> > > 
> > > > then it's easy enough to specify - and we can make it the zero/default
> > > > value too.
> > > 
> > > OK. Then we will make MIX as zero/default and add "allowed_event_types" in
> > > event queue config.
> > >
> > 
> > Bruce,
> > 
> > I have tried to make it as "allowed_event_types" in event queue config.
> > However, rte_event_queue_default_conf_get() can also take NULL for default
> > configuration. So I think, It makes sense to go with negation approach
> > like ethdev to define the default to avoid confusion on the default. So
> > I am thinking like below now,
> > 
> > ➜ [master][libeventdev] $ git diff
> > diff --git a/rte_eventdev.h b/rte_eventdev.h
> > index cf22b0e..cac4642 100644
> > --- a/rte_eventdev.h
> > +++ b/rte_eventdev.h
> > @@ -429,6 +429,12 @@ rte_event_dev_configure(uint8_t dev_id, struct
> > rte_event_dev_config *config);
> >   *
> >   *  \see rte_event_port_setup(), rte_event_port_link()
> >   */
> > +#define RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE      (1ULL << 1)
> > +/**< Skip configuring atomic schedule type resources */
> > +#define RTE_EVENT_QUEUE_CFG_NOORDERED_TYPE     (1ULL << 2)
> > +/**< Skip configuring ordered schedule type resources */
> > +#define RTE_EVENT_QUEUE_CFG_NOPARALLEL_TYPE    (1ULL << 3)
> > +/**< Skip configuring parallel schedule type resources */
> > 
> >  /** Event queue configuration structure */
> >  struct rte_event_queue_conf {
> > 
> > Thoughts?
> > 
> 
> I'm ok with the default as being all types, in the case where NULL is
> specified for the parameter. It does make the most sense.

Yes. That case I need to explicitly mention in the documentation about what
is default case. With RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE scheme it quite
understood what is default. Not adding up? :-)

> 
> However, for the cases where the user does specify what they want, I
> think it does make more sense, and is easier on the user for things to
> be specified in a positive, rather than negative sense. For a user who
> wants to just use atomic events, having to specify that as "not-reordered
> and not-unordered" just isn't as clear! :-)
> 
> /Bruce
>
  

On Wed, Nov 02, 2016 at 06:39:27PM +0530, Jerin Jacob wrote:
> On Wed, Nov 02, 2016 at 11:35:51AM +0000, Bruce Richardson wrote:
> > On Wed, Nov 02, 2016 at 04:55:22PM +0530, Jerin Jacob wrote:
> > > On Fri, Oct 28, 2016 at 02:36:48PM +0530, Jerin Jacob wrote:
> > > > On Fri, Oct 28, 2016 at 09:36:46AM +0100, Bruce Richardson wrote:
> > > > > On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> > > > > > On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > > > > > > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > > > > How about making default as "mixed" and let application configures what
> > > > > > is not required?. That way application responsibility is clear.
> > > > > > something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> > > > > > with default.
> > > > > > 
> > > > > I suppose it could work, but why bother doing that? If an app knows it's
> > > > > only going to use one traffic type, why not let it just state what it
> > > > > will do rather than try to specify what it won't do. If mixed is needed,
> > > > 
> > > > My thought was more inline with ethdev spec, like, ref-count is default,
> > > > if application need exception then set ETH_TXQ_FLAGS_NOREFCOUNT. But it is OK, if
> > > > you need other way.
> > > > 
> > > > > then it's easy enough to specify - and we can make it the zero/default
> > > > > value too.
> > > > 
> > > > OK. Then we will make MIX as zero/default and add "allowed_event_types" in
> > > > event queue config.
> > > >
> > > 
> > > Bruce,
> > > 
> > > I have tried to make it as "allowed_event_types" in event queue config.
> > > However, rte_event_queue_default_conf_get() can also take NULL for default
> > > configuration. So I think, It makes sense to go with negation approach
> > > like ethdev to define the default to avoid confusion on the default. So
> > > I am thinking like below now,
> > > 
> > > ➜ [master][libeventdev] $ git diff
> > > diff --git a/rte_eventdev.h b/rte_eventdev.h
> > > index cf22b0e..cac4642 100644
> > > --- a/rte_eventdev.h
> > > +++ b/rte_eventdev.h
> > > @@ -429,6 +429,12 @@ rte_event_dev_configure(uint8_t dev_id, struct
> > > rte_event_dev_config *config);
> > >   *
> > >   *  \see rte_event_port_setup(), rte_event_port_link()
> > >   */
> > > +#define RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE      (1ULL << 1)
> > > +/**< Skip configuring atomic schedule type resources */
> > > +#define RTE_EVENT_QUEUE_CFG_NOORDERED_TYPE     (1ULL << 2)
> > > +/**< Skip configuring ordered schedule type resources */
> > > +#define RTE_EVENT_QUEUE_CFG_NOPARALLEL_TYPE    (1ULL << 3)
> > > +/**< Skip configuring parallel schedule type resources */
> > > 
> > >  /** Event queue configuration structure */
> > >  struct rte_event_queue_conf {
> > > 
> > > Thoughts?
> > > 
> > 
> > I'm ok with the default as being all types, in the case where NULL is
> > specified for the parameter. It does make the most sense.
> 
> Yes. That case I need to explicitly mention in the documentation about what
> is default case. With RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE scheme it quite
> understood what is default. Not adding up? :-)
> 

Would below not work? DEFAULT explicitly stated, and can be commented to
say all types allowed.

#define RTE_EVENT_QUEUE_CFG_DEFAULT 0
#define RTE_EVENT_QUEUE_CFG_ALL_TYPES RTE_EVENT_QUEUE_CFG_DEFAULT
#define RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY (1<<0)
#define RTE_EVENT_QUEUE_CFG_ORDERED_ONLY (1<<1) 
....

/Bruce
  

On Wed, Nov 02, 2016 at 01:56:27PM +0000, Bruce Richardson wrote:
> On Wed, Nov 02, 2016 at 06:39:27PM +0530, Jerin Jacob wrote:
> > On Wed, Nov 02, 2016 at 11:35:51AM +0000, Bruce Richardson wrote:
> > > On Wed, Nov 02, 2016 at 04:55:22PM +0530, Jerin Jacob wrote:
> > > > On Fri, Oct 28, 2016 at 02:36:48PM +0530, Jerin Jacob wrote:
> > > > > On Fri, Oct 28, 2016 at 09:36:46AM +0100, Bruce Richardson wrote:
> > > > > > On Fri, Oct 28, 2016 at 08:31:41AM +0530, Jerin Jacob wrote:
> > > > > > > On Wed, Oct 26, 2016 at 01:54:14PM +0100, Bruce Richardson wrote:
> > > > > > > > On Wed, Oct 26, 2016 at 05:54:17PM +0530, Jerin Jacob wrote:
> > > > > > > How about making default as "mixed" and let application configures what
> > > > > > > is not required?. That way application responsibility is clear.
> > > > > > > something similar to ETH_TXQ_FLAGS_NOMULTSEGS, ETH_TXQ_FLAGS_NOREFCOUNT
> > > > > > > with default.
> > > > > > > 
> > > > > > I suppose it could work, but why bother doing that? If an app knows it's
> > > > > > only going to use one traffic type, why not let it just state what it
> > > > > > will do rather than try to specify what it won't do. If mixed is needed,
> > > > > 
> > > > > My thought was more inline with ethdev spec, like, ref-count is default,
> > > > > if application need exception then set ETH_TXQ_FLAGS_NOREFCOUNT. But it is OK, if
> > > > > you need other way.
> > > > > 
> > > > > > then it's easy enough to specify - and we can make it the zero/default
> > > > > > value too.
> > > > > 
> > > > > OK. Then we will make MIX as zero/default and add "allowed_event_types" in
> > > > > event queue config.
> > > > >
> > > > 
> > > > Bruce,
> > > > 
> > > > I have tried to make it as "allowed_event_types" in event queue config.
> > > > However, rte_event_queue_default_conf_get() can also take NULL for default
> > > > configuration. So I think, It makes sense to go with negation approach
> > > > like ethdev to define the default to avoid confusion on the default. So
> > > > I am thinking like below now,
> > > > 
> > > > ➜ [master][libeventdev] $ git diff
> > > > diff --git a/rte_eventdev.h b/rte_eventdev.h
> > > > index cf22b0e..cac4642 100644
> > > > --- a/rte_eventdev.h
> > > > +++ b/rte_eventdev.h
> > > > @@ -429,6 +429,12 @@ rte_event_dev_configure(uint8_t dev_id, struct
> > > > rte_event_dev_config *config);
> > > >   *
> > > >   *  \see rte_event_port_setup(), rte_event_port_link()
> > > >   */
> > > > +#define RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE      (1ULL << 1)
> > > > +/**< Skip configuring atomic schedule type resources */
> > > > +#define RTE_EVENT_QUEUE_CFG_NOORDERED_TYPE     (1ULL << 2)
> > > > +/**< Skip configuring ordered schedule type resources */
> > > > +#define RTE_EVENT_QUEUE_CFG_NOPARALLEL_TYPE    (1ULL << 3)
> > > > +/**< Skip configuring parallel schedule type resources */
> > > > 
> > > >  /** Event queue configuration structure */
> > > >  struct rte_event_queue_conf {
> > > > 
> > > > Thoughts?
> > > > 
> > > 
> > > I'm ok with the default as being all types, in the case where NULL is
> > > specified for the parameter. It does make the most sense.
> > 
> > Yes. That case I need to explicitly mention in the documentation about what
> > is default case. With RTE_EVENT_QUEUE_CFG_NOATOMIC_TYPE scheme it quite
> > understood what is default. Not adding up? :-)
> > 
> 
> Would below not work? DEFAULT explicitly stated, and can be commented to
> say all types allowed.

All I was trying to avoid explicitly stating the default state. Not worth
to have back and forth on slow path configuration, I will keep it as
positive logic as you suggested :-) and inspired from PKT_TX_L4_MASK

#define RTE_EVENT_QUEUE_CFG_TYPE_MASK       (3ULL << 0)
#define RTE_EVENT_QUEUE_CFG_ALL_TYPES       (0ULL << 0) /**< Enable all types */
#define RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY     (1ULL << 0)
#define RTE_EVENT_QUEUE_CFG_ORDERED_ONLY    (2ULL << 0)
#define RTE_EVENT_QUEUE_CFG_PARALLEL_ONLY   (3ULL << 0)
#define RTE_EVENT_QUEUE_CFG_SINGLE_CONSUMER (1ULL << 2)

> 
> #define RTE_EVENT_QUEUE_CFG_DEFAULT 0
> #define RTE_EVENT_QUEUE_CFG_ALL_TYPES RTE_EVENT_QUEUE_CFG_DEFAULT
> #define RTE_EVENT_QUEUE_CFG_ATOMIC_ONLY (1<<0)
> #define RTE_EVENT_QUEUE_CFG_ORDERED_ONLY (1<<1) 
> ....
> 
> /Bruce