[sheepdog] [PATCH] lib/work: initialize `tid_max` to 1
Jinzhi Chen
nxtjinzhi at gmail.com
Fri Oct 31 05:40:19 CET 2014
sorry, above patch is wrong.
I'll resend it later.
On Fri, Oct 31, 2014 at 10:12 AM, Jinzhi Chen <nxtjinzhi at gmail.com> wrote:
> [PATCH] Initialize trace before journal init
>
> when journal thread init before the initialization of trace,
> it enter into loop (using uninitailized `tid_max`)with
> --debug-enable configured. we need to initialize trace before
> journal thread init.
> this patch extrace `wq_trace_init` from `init_work_queue` and
> call it in the main() function just before the initialization
> of journal file.
> ---
> lib/work.c | 48 ++++++++++++++++++++++--------------------------
> sheep/sheep.c | 9 +++++++++
> 2 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/lib/work.c b/lib/work.c
> index 7e2cd79..e36516a 100644
> --- a/lib/work.c
> +++ b/lib/work.c
> @@ -70,6 +70,7 @@ static size_t nr_nodes = 1;
> static size_t (*wq_get_nr_nodes)(void);
>
> static void *worker_routine(void *arg);
> +int wq_trace_init (void);
>
> #ifdef HAVE_TRACE
>
> @@ -134,27 +135,6 @@ static void suspend(int num)
> eventfd_xwrite(ack_efd, 1); /* ack of resume */
> }
>
> -static int wq_trace_init(void)
> -{
> - tid_max = TID_MAX_DEFAULT;
> - tid_map = alloc_bitmap(NULL, 0, tid_max);
> -
> - resume_efd = eventfd(0, EFD_SEMAPHORE);
> - ack_efd = eventfd(0, EFD_SEMAPHORE);
> -
> - if (resume_efd < 0 || ack_efd < 0) {
> - sd_err("failed to create event fds: %m");
> - return -1;
> - }
> -
> - /* trace uses this signal to suspend the worker threads */
> - if (install_sighandler(SIGUSR2, suspend, false) < 0) {
> - sd_debug("%m");
> - return -1;
> - }
> - return 0;
> -}
> -
> static void trace_set_tid_map(int tid)
> {
> sd_mutex_lock(&tid_map_lock);
> @@ -180,12 +160,32 @@ static void trace_clear_tid_map(int tid)
>
> #else
>
> -static inline int wq_trace_init(void) { return 0; }
> static inline void trace_set_tid_map(int tid) {}
> static inline void trace_clear_tid_map(int tid) {}
>
> #endif /* HAVE_TRACE */
>
> +int wq_trace_init(void)
> +{
> + tid_max = TID_MAX_DEFAULT;
> + tid_map = alloc_bitmap(NULL, 0, tid_max);
> +
> + resume_efd = eventfd(0, EFD_SEMAPHORE);
> + ack_efd = eventfd(0, EFD_SEMAPHORE);
> +
> + if (resume_efd < 0 || ack_efd < 0) {
> + sd_err("failed to create event fds: %m");
> + return -1;
> + }
> +
> + /* trace uses this signal to suspend the worker threads */
> + if (install_sighandler(SIGUSR2, suspend, false) < 0) {
> + sd_debug("%m");
> + return -1;
> + }
> + return 0;
> +}
> +
> static uint64_t get_msec_time(void)
> {
> struct timeval tv;
> @@ -364,10 +364,6 @@ int init_work_queue(size_t (*get_nr_nodes)(void))
> return -1;
> }
>
> - ret = wq_trace_init();
> - if (ret < 0)
> - return ret;
> -
> ret = register_event(efd, worker_thread_request_done, NULL);
> if (ret) {
> sd_err("failed to register event fd %m");
> diff --git a/sheep/sheep.c b/sheep/sheep.c
> index 2e91d0f..071e467 100644
> --- a/sheep/sheep.c
> +++ b/sheep/sheep.c
> @@ -148,6 +148,8 @@ static struct sd_option sheep_options[] = {
> { 0, NULL, false, NULL },
> };
>
> +extern int wq_trace_init (void);
> +
> static void usage(int status)
> {
> if (status) {
> @@ -885,6 +887,13 @@ int main(int argc, char **argv)
> goto cleanup_log;
> }
>
> + /* We should init trace before journal init */
> + ret = wq_trace_init ();
> + if (ret) {
> + sd_err("failed to init trace");
> + goto cleanup_log;
> + }
> +
> /* We should init journal file before backend init */
> if (uatomic_is_true(&sys->use_journal)) {
> if (!strlen(jpath))
> --
> 1.7.9.5
>
>
> On Thu, Oct 30, 2014 at 4:14 PM, Hitoshi Mitake <mitake.hitoshi at gmail.com>
> wrote:
>
>> On Thu, Oct 30, 2014 at 2:46 PM, Jinzhi Chen <nxtjinzhi at gmail.com> wrote:
>> > thank you for your reply.
>> > it sounds good to me.
>> >
>> > how about extract `wq_trace_init` from `init_work_queue` (lib/work.c)
>> and
>> > call it in the main() function just before the initialization of journal
>> > file.
>> > we can initialize work queue trace related variable before the
>> > initialization of
>> > work queue. In this manner, we don't need a new function.
>>
>> Seems good. Could you prepare v2 with the above scheme?
>>
>> Thanks,
>> Hitoshi
>>
>> >
>> >
>> > Thanks
>> > Jinzhi Chen
>> >
>> > On Thu, Oct 30, 2014 at 9:42 AM, Hitoshi Mitake
>> > <mitake.hitoshi at lab.ntt.co.jp> wrote:
>> >>
>> >>
>> >> Hi Jinzhi, thanks for your patch.
>> >>
>> >> At Wed, 29 Oct 2014 13:53:14 +0800,
>> >> Jinzhi Chen wrote:
>> >> >
>> >> > [1 <multipart/alternative (7bit)>]
>> >> > [1.1 <text/plain; UTF-8 (7bit)>]
>> >> > journal thread uses `tid_max` which is not initialized until
>> >> > later initial progress. The default value 0 cause journal
>> >> > thread loop in the function `trace_set_tid_map` , so set
>> >> > default value to 1.
>> >> >
>> >> > Signed-off-by: jinzhichen <nxtjinzhi at gmail.com>
>> >>
>> >> Could you write your name "Jinzhi Chen" instead of jinzhichen in
>> >> Signed-off-by: line?
>> >>
>> >> > ---
>> >> > lib/work.c | 2 +-
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/lib/work.c b/lib/work.c
>> >> > index 7e2cd79..7b20685 100644
>> >> > --- a/lib/work.c
>> >> > +++ b/lib/work.c
>> >> > @@ -75,7 +75,7 @@ static void *worker_routine(void *arg);
>> >> >
>> >> > #define TID_MAX_DEFAULT 0x8000 /* default maximum tid for most
>> systems
>> >> > */
>> >> >
>> >> > -static size_t tid_max;
>> >> > +static size_t tid_max = 1;
>> >>
>> >> How about the below way? Extracting the code
>> >>
>> >> tid_max = TID_MAX_DEFAULT;
>> >> tid_map = alloc_bitmap(NULL, 0, tid_max);
>> >>
>> >> from wq_trace_init(), prepare a new function,
>> >> e.g. early_trace_init(), which does the above initialization, and let
>> >> main() call the function in its early part. Practically your patch can
>> >> solve the problem but the above way will not produce a combination of
>> >> initialized tid_max and uninitialized tid_map. How do you think?
>> >>
>> >> Thanks,
>> >> Hitoshi
>> >>
>> >> > static unsigned long *tid_map;
>> >> > static struct sd_mutex tid_map_lock = SD_MUTEX_INITIALIZER;
>> >> >
>> >> > --
>> >> > 1.7.9.5
>> >> > [1.2 <text/html; UTF-8 (quoted-printable)>]
>> >> >
>> >> > [2 <text/plain; us-ascii (7bit)>]
>> >> > --
>> >> > sheepdog mailing list
>> >> > sheepdog at lists.wpkg.org
>> >> > http://lists.wpkg.org/mailman/listinfo/sheepdog
>> >
>> >
>> >
>> > --
>> > sheepdog mailing list
>> > sheepdog at lists.wpkg.org
>> > http://lists.wpkg.org/mailman/listinfo/sheepdog
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wpkg.org/pipermail/sheepdog/attachments/20141031/09f3f783/attachment-0004.html>
More information about the sheepdog
mailing list