nfs-ganesha icon indicating copy to clipboard operation
nfs-ganesha copied to clipboard

Thread termination timeout in ganesha

Open skmprabhu1 opened this issue 2 years ago • 7 comments

Considering clock speed of most of current processors, I feel 31 seconds is huge. Can we reduce thread termination timeout to 10-15 seconds or 5 secs?

#define WORK_POOL_TIMEOUT_MS (31 /* seconds (prime) */ * 1000)

pool->timeout_ms = WORK_POOL_TIMEOUT_MS;

            clock_gettime(CLOCK_REALTIME_FAST, &ts);
            timespec_addms(&ts, pool->timeout_ms);

            /* Note: the mutex is the pool _head,
             * but the condition is per worker,
             * making the signal efficient!
             */
            rc = pthread_cond_timedwait(&wpt->pqcond, &pool->pqh.qmutex, &ts);

skmprabhu1 avatar Jun 30 '22 19:06 skmprabhu1

All this timeout does is determine how long we have to wait with no work before we will reduce the number of threads in the pool. I don't think 31 seconds is unreasonable for that, but I also don't have a very strong opinion on it.

dang avatar Jul 01 '22 13:07 dang

one more question related to thread pool management.

From work_pool_thread()

            if (0 > pool->pqh.qcount++) {
                    /* negative for task(s) */
                    have = TAILQ_FIRST(&pool->pqh.qh);
                    TAILQ_REMOVE(&pool->pqh.qh, have, q);

                    wpt->work = (struct work_pool_entry *)have;
                    continue;
            }

            /* positive for waiting worker(s):
             * use the otherwise empty pool to hold them,
             * simplifying mutex and pointer setup.
             */
            TAILQ_INSERT_TAIL(&pool->pqh.qh, &wpt->pqe, q);

Based on the above code, we are removing thread from the queue head and assigning to new work. And, we are adding idle thread (work completed thread) to tail.

Lets say if the server is too busy (in peak hours) and created max number of threads( 512 by default); then later if server receives request with regular interval (lets say 1/20 secs) , we may end of with running all 512 thread all the time. That's thread will get assigned to new work before its timeout (31secs). The situation may be worse if the server configured with huge number of threads(like 10K threads).

should we consider recent idle thread for new work (that's either get idle using TAILQ_LAST or add idle in TAILQ_INSERT_HEAD) ?

Please let us know your thought.

skmprabhu1 avatar Jul 01 '22 13:07 skmprabhu1

All this timeout does is determine how long we have to wait with no work before we will reduce the number of threads in the pool. I don't think 31 seconds is unreasonable for that, but I also don't have a very strong opinion on it.

Thanks Daniel. If there is no issue, can we reduce timeout to 10 secs?

skmprabhu1 avatar Jul 01 '22 14:07 skmprabhu1

A shorter timeout seems like it would be OK. I don't think thread creation is THAT expensive an operation?

ffilz avatar Jul 05 '22 17:07 ffilz

Thanks Frank. I'll change timeout to 10 secs and submit patch.

Yes. Thread creation time is not expensive.

Few customers asked why Ganesha is running with lot of threads(after peak workload) even though not much workload from NFS client. Reducing timeout should improve situation because we may discard idle thread much faster.

skmprabhu1 avatar Sep 21 '22 19:09 skmprabhu1

Is this issue still relevant? We would entertain a patch for this.

ffilz avatar Jan 24 '23 19:01 ffilz