I see Petri, I'll resend with iquery patchset together. Thanks and Best Regards, Yi
On 8 December 2016 at 15:52, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolai...@nokia-bell-labs.com> wrote: > Hi, > > The point is that it cannot be reviewed as stand alone, since reviwer does > not see the reason why it's needed. Other two schedulers do not need it, > since those do not call dequeue simultaneously from multiple threads for > the same queue. > > -Petri > > > From: Yi He [mailto:yi...@linaro.org] > Sent: Thursday, December 08, 2016 9:40 AM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- > labs.com> > Cc: lng-odp@lists.linaro.org; bill.fischo...@linaro.org; Elo, Matias > (Nokia - FI/Espoo) <matias....@nokia-bell-labs.com> > Subject: Re: [lng-odp] [API-NEXT PATCH] linux-gen: scheduler: solve > ordered context inversion > > Hi, Petri > > I think this patch can stand alone for the problem it explains and solves, > the two actions (dequeue events and get context sequence) are non-atomic > and preemptable. > > This patch suggest that in queue dequeue operation, to add a > sched->save_context() callback in an atomic manner. > > So lately default scheduler and SP scheduler can also move the context > saving code into this call back, what do you think? > > Best Regards, Yi > > On 8 December 2016 at 15:29, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolai...@nokia-bell-labs.com> wrote: > This patch belongs to the iquery scheduler patch set. Without the rest of > the code, it's impossible to tell if this change is needed or correct. > > -Petri > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Yi > He > > Sent: Thursday, December 08, 2016 8:33 AM > > To: lng-odp@lists.linaro.org; bill.fischo...@linaro.org; Elo, Matias > > (Nokia - FI/Espoo) <matias....@nokia-bell-labs.com> > > Subject: [lng-odp] [API-NEXT PATCH] linux-gen: scheduler: solve ordered > > context inversion > > > > For ordered queue, a thread consumes events (dequeue) and > > acquires its unique sequential context in two steps, non > > atomic and preemptable. > > > > This leads to potential ordered context inversion in case > > the thread consumes prior events acquired subsequent context, > > while the thread consumes subsequent events but acquired > > prior context. > > > > This patch insert the ordered context acquisition into > > event dequeue operation to make these two steps atomic. > > > > Signed-off-by: Yi He <yi...@linaro.org> > > --- > > When I'm rebase iquery scheduler onto Matias' new ordered > > queue impl I found this potential ordered context inversion > > issue, it didnot happen to the default scheduler but happens > > to the iquery scheduler, so after patchset will rely on this > > patch. please help review thanks. > > > > platform/linux-generic/include/odp_schedule_if.h | 3 +++ > > platform/linux-generic/odp_queue.c | 3 +++ > > platform/linux-generic/odp_schedule.c | 7 ++++++- > > platform/linux-generic/odp_schedule_sp.c | 7 ++++++- > > 4 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/platform/linux-generic/include/odp_schedule_if.h > > b/platform/linux-generic/include/odp_schedule_if.h > > index 6c2b050..c0aee42 100644 > > --- a/platform/linux-generic/include/odp_schedule_if.h > > +++ b/platform/linux-generic/include/odp_schedule_if.h > > @@ -12,6 +12,7 @@ extern "C" { > > #endif > > > > #include <odp/api/queue.h> > > +#include <odp_queue_internal.h> > > #include <odp/api/schedule.h> > > > > typedef void (*schedule_pktio_start_fn_t)(int pktio_index, int > > num_in_queue, > > @@ -33,6 +34,7 @@ typedef int (*schedule_term_local_fn_t)(void); > > typedef void (*schedule_order_lock_fn_t)(void); > > typedef void (*schedule_order_unlock_fn_t)(void); > > typedef unsigned (*schedule_max_ordered_locks_fn_t)(void); > > +typedef void (*schedule_save_context_fn_t)(queue_entry_t *queue); > > > > typedef struct schedule_fn_t { > > schedule_pktio_start_fn_t pktio_start; > > @@ -50,6 +52,7 @@ typedef struct schedule_fn_t { > > schedule_order_lock_fn_t order_lock; > > schedule_order_unlock_fn_t order_unlock; > > schedule_max_ordered_locks_fn_t max_ordered_locks; > > + schedule_save_context_fn_t save_context; > > } schedule_fn_t; > > > > /* Interface towards the scheduler */ > > diff --git a/platform/linux-generic/odp_queue.c b/platform/linux- > > generic/odp_queue.c > > index d9cb9f3..3b6a68b 100644 > > --- a/platform/linux-generic/odp_queue.c > > +++ b/platform/linux-generic/odp_queue.c > > @@ -568,6 +568,9 @@ static inline int deq_multi(queue_entry_t *queue, > > odp_buffer_hdr_t *buf_hdr[], > > if (hdr == NULL) > > queue->s.tail = NULL; > > > > + if (queue->s.type == ODP_QUEUE_TYPE_SCHED) > > + sched_fn->save_context(queue); > > + > > UNLOCK(&queue->s.lock); > > > > return i; > > diff --git a/platform/linux-generic/odp_schedule.c b/platform/linux- > > generic/odp_schedule.c > > index 645630a..264c58f 100644 > > --- a/platform/linux-generic/odp_schedule.c > > +++ b/platform/linux-generic/odp_schedule.c > > @@ -1205,6 +1205,10 @@ static int schedule_num_grps(void) > > return NUM_SCHED_GRPS; > > } > > > > +static void schedule_save_context(queue_entry_t *queue ODP_UNUSED) > > +{ > > +} > > + > > /* Fill in scheduler interface */ > > const schedule_fn_t schedule_default_fn = { > > .pktio_start = schedule_pktio_start, > > @@ -1221,7 +1225,8 @@ const schedule_fn_t schedule_default_fn = { > > .term_local = schedule_term_local, > > .order_lock = order_lock, > > .order_unlock = order_unlock, > > - .max_ordered_locks = schedule_max_ordered_locks > > + .max_ordered_locks = schedule_max_ordered_locks, > > + .save_context = schedule_save_context > > }; > > > > /* Fill in scheduler API calls */ > > diff --git a/platform/linux-generic/odp_schedule_sp.c b/platform/linux- > > generic/odp_schedule_sp.c > > index 76d1357..8481f29 100644 > > --- a/platform/linux-generic/odp_schedule_sp.c > > +++ b/platform/linux-generic/odp_schedule_sp.c > > @@ -676,6 +676,10 @@ static void order_unlock(void) > > { > > } > > > > +static void save_context(queue_entry_t *queue ODP_UNUSED) > > +{ > > +} > > + > > /* Fill in scheduler interface */ > > const schedule_fn_t schedule_sp_fn = { > > .pktio_start = pktio_start, > > @@ -692,7 +696,8 @@ const schedule_fn_t schedule_sp_fn = { > > .term_local = term_local, > > .order_lock = order_lock, > > .order_unlock = order_unlock, > > - .max_ordered_locks = max_ordered_locks > > + .max_ordered_locks = max_ordered_locks, > > + .save_context = save_context > > }; > > > > /* Fill in scheduler API calls */ > > -- > > 2.7.4 > >
