On Thu, Aug 16, 2018 at 07:31:17PM +0200, Willy Tarreau wrote:
> Both patches applied, thanks guys!
> 
> Olivier, I have a suggestion for this one :
> On Thu, Aug 16, 2018 at 07:17:07PM +0200, Olivier Houchard wrote:
> > From 90fc92f777772c6b47d88769bb73680702d7b8e6 Mon Sep 17 00:00:00 2001
> > From: Olivier Houchard <ohouch...@haproxy.com>
> > Date: Thu, 16 Aug 2018 19:03:02 +0200
> > Subject: [PATCH 1/2] BUG/MEDIUM: tasks: Don't insert in the global rqueue if
> >  nbthread == 1
> > 
> > Make sure we don't insert a task in the global run queue if nbthread == 1,
> > as, as an optimisation, we avoid reading from it if nbthread == 1.
> > ---
> >  src/task.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/task.c b/src/task.c
> > index de097baf7..e357bc169 100644
> > --- a/src/task.c
> > +++ b/src/task.c
> > @@ -395,7 +395,8 @@ void process_runnable_tasks()
> >                     state = HA_ATOMIC_AND(&t->state, ~TASK_RUNNING);
> >                     if (state)
> >  #ifdef USE_THREAD
> > -                           __task_wakeup(t, (t->thread_mask == tid_bit) ?
> > +                           __task_wakeup(t, (t->thread_mask == tid_bit ||
> > +                               global.nbthread == 1) ?
> >                                 &rqueue_local[tid] : &rqueue);
> >  #else
> 
> I'm pretty sure we'll get caught by this sort of stuff in the future
> again. I think it would be safer to proceed like this :
> 
>      __task_wakeup(t, ((t->thread_mask & all_threads_mask) == tid_bit)
> 
> It should always be valid regardless of the number of threads. The same
> could be done in task_wakeup().
> 

Sure, why not.

> I suspect we may have a similar issue with the fd cache applied to listeners
> here :
> 
>    static inline void updt_fd_polling(const int fd)
>    {
>         if (fdtab[fd].thread_mask == tid_bit) {
>                 unsigned int oldupdt;
> 
>                 /* note: we don't have a test-and-set yet in hathreads */
> 
> 
> In this case the thread_mask might be larger than all_threads_mask and
> we might fail this test. Or we may see that when threads exit and the
> threads mask changes.
> 
> Just my two cents, both patches were applied anyway.
> 

That is true, this one is not a bug, but a pessimization, by using the global
update_list which is more costly than the local one.

Patches attached to do as suggest.

Regards,

Olivier

>From c61f3f76f7e546aac14d7db33552dba11ce71e12 Mon Sep 17 00:00:00 2001
From: Olivier Houchard <ohouch...@haproxy.com>
Date: Fri, 17 Aug 2018 13:36:08 +0200
Subject: [PATCH 1/2] MINOR: tasks: Don't special-case when nbthreads == 1

Instead of checking if nbthreads == 1, just and thread_mask with
all_threads_mask to know if we're supposed to add the task to the local or
the global runqueue.
---
 src/task.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/task.c b/src/task.c
index e357bc169..ce5b4f907 100644
--- a/src/task.c
+++ b/src/task.c
@@ -395,8 +395,7 @@ void process_runnable_tasks()
                        state = HA_ATOMIC_AND(&t->state, ~TASK_RUNNING);
                        if (state)
 #ifdef USE_THREAD
-                               __task_wakeup(t, (t->thread_mask == tid_bit ||
-                                   global.nbthread == 1) ?
+                               __task_wakeup(t, ((t->thread_mask & 
all_threads_mask) == tid_bit) ?
                                    &rqueue_local[tid] : &rqueue);
 #else
                                __task_wakeup(t, &rqueue_local[tid]);
-- 
2.14.3

>From ea3b07fd2ed96b46ea107c9d862b5a1c27e728c2 Mon Sep 17 00:00:00 2001
From: Olivier Houchard <ohouch...@haproxy.com>
Date: Fri, 17 Aug 2018 13:37:59 +0200
Subject: [PATCH 2/2] MINOR: fd cache: And the thread_mask with
 all_threads_mask.

When we choose to insert a fd in either the global or the local fd update list,
and the thread_mask against all_threads_mask before checking if it's tid_bit,
that way, if we run with nbthreads==1, we will always use the local list,
which is cheaper than the global one.
---
 include/proto/fd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/proto/fd.h b/include/proto/fd.h
index a4cee3220..a3ec5e854 100644
--- a/include/proto/fd.h
+++ b/include/proto/fd.h
@@ -109,7 +109,7 @@ void fd_rm_from_fd_list(volatile struct fdlist *list, int 
fd, int off);
  */
 static inline void updt_fd_polling(const int fd)
 {
-       if (fdtab[fd].thread_mask == tid_bit) {
+       if ((fdtab[fd].thread_mask & all_threads_mask) == tid_bit) {
                unsigned int oldupdt;
 
                /* note: we don't have a test-and-set yet in hathreads */
-- 
2.14.3

Reply via email to