Re: [dpdk-dev] [PATCH 1/3] evendev: fix inconsistency in event queue config

[Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>]

On Fri, Oct 20, 2017 at 04:38:57PM +0000, Van Haaren, Harry wrote:
> > From: Pavan Nikhilesh Bhagavatula [mailto:pbhagavatula@caviumnetworks.com]
> > Sent: Friday, October 20, 2017 11:31 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 1/3] evendev: fix inconsistency in event
> > queue config
> >
> > On Fri, Oct 20, 2017 at 09:54:36AM +0000, Van Haaren, Harry wrote:
> > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> > > > Sent: Thursday, October 12, 2017 2:16 PM
> > > > To: jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; Van Haaren,
> > > > Harry <harry.van.haaren@intel.com>
> > > > Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > Subject: [dpdk-dev] [PATCH 1/3] evendev: fix inconsistency in event
> > queue
> > > > config
> > > >

<snip>

> > > > -	}
> > > > -	return ret;
> > > > -}
> > >
> > >
> > > We should note here, that SCHED_TYPE are integer values:
> > > #define RTE_SCHED_TYPE_ORDERED          0
> > > #define RTE_SCHED_TYPE_ATOMIC           1
> > > #define RTE_SCHED_TYPE_PARALLEL         2
> > >
> > > While the EVENT_QUEUE_CFG_ types were bitmasks (before being removed in
> > this patch)
> > > #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_LINK        (1ULL << 2)
> > >
> > >
> > > I'm not against this change - but we must be careful that if there was any
> > bit-masking being used previously,
> > > that that will subtly have broken if we change to sched types. I'm
> > reviewing with that in mind..
> > >
> > > The RTE_EVENT_QUEUE_CFG_ALL_TYPES  config flag now means that all
> > SCHED_TYPEs
> > > are valid. Previously this was contained in the bitmask.. this may lead to
> > issues.
> > >
> > > See patch 2/3, where *only* the schedule_type is read, and returned. What
> > if it the "ALL_TYPES" flag is
> > > set on the queue? It will be reported wrongly. Currently there is no
> > integer value for "ALL_TYPES",
> > > so we cannot represent "ALL TYPES" in the return value from get_attr().
> > >
> > > Am I explaining my reasoning clearly enough?
> > >
> >
> > Hey Harry,
> >
> > I do understand what you mean, my initial thought was to include "ALL_TYPES"
> > as
> > a schedule_type in queue config but this would just complicate things.
> >
> > As these values are only used in config phase we could have a check to see
> > if
> > event_queue_cfg is not "ALL_TYPES" and only then return the value of
> > sched_type
> > else return a error value in case of get_attr().
> >
> > I think most of the places this specific check is handled, one such missed
> > place would be get_attr(). If we could come to a conclusion to fix it in a
> > correct way I will send out a v2.
>
>
> Sure, I see two sane-ish options:
>
> 1) Return an error code from get_attr(), which actually means "ALL TYPES". Feels a bit weird, because an error value is really a valid return.
>
> 2) Return UINT_MAX (aka, -1) as the scheduling value. Applications that use/care about the scheduling type must check, others can ignore it.
>
> I'm not sure which of these is the better/less-bad solution. Opinions? -H
>

I think 1st option would be good, we could use ENOTUNIQ to represent that the
queue type is "ALL TYPE".

Thoughts?

Pavan.