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 <[email protected]>
> > 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 <[email protected]>
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 <[email protected]>
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
