Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 30 Dec 2013 11:31:14 +0100
From:      Mike Ma <mikemandarine@gmail.com>
To:        Mark Johnston <markj@freebsd.org>
Cc:        "dtrace@freebsd.org" <dtrace@freebsd.org>
Subject:   Re: [CFT] Use vmem in dtrace
Message-ID:  <CA%2BcqwfdBKY7Y9t3RyLtQNAYwiDT=PPD0dyeoe4Uk-iXUdTk3dg@mail.gmail.com>
In-Reply-To: <20131230024945.GA23245@charmander.home>
References:  <CA%2Bcqwfe_v_XKdYGd-ocbEg4Ey5QHwRW1KoxSx5BAY0BPKMg6sw@mail.gmail.com> <20131230024945.GA23245@charmander.home>

next in thread | previous in thread | raw e-mail | index | archive | help
Hi Mark,

Thanks a lot for your help.
It works fine for me now as well.
Please commit it then.

Thanks a lot.

On Mon, Dec 30, 2013 at 3:52 AM, Mark Johnston <markj@freebsd.org> wrote:
> On Fri, Dec 13, 2013 at 08:17:03AM +0100, Mike Ma wrote:
>> Hi there,
>>
>> I'm a GSoC student this year, and Pedro guided me to dtrace recently.
>>
>> I'm sending my patch for a vmem task listed in the TODO list (12th item),
>> as there is vmem subsystem in FreeBSD 10/11 available now.
>> Basically, it is to revert changes from this patch
>> http://lists.freebsd.org/pipermail/p4-projects/2008-January/023466.html
>> And the main difference is that vmem_create and vmem_alloc take different
>> number of parameters on FreeBSD.
>>
>> I did run dtrace testsuite on my own machine.
>> And I hope someone here can try my patch.
>
> I've placed a modified form of your patch here:
> http://people.freebsd.org/~markj/patches/dtrace_vmem.diff
>
> I had to changes a few things for it to work for me:
> - vmem_alloc returns resources in its last argument, which cannot be
>   NULL (else one invariably gets a page fault on line 1130 of subr_vmem.c).
> - the probe id arena should be created before the kld_* event handlers
>   are registered, since they call dtrace_probe_create(), which uses the
>   arena.
> - for some reason, the snprintf in dtrace_state_create() was placed
>   under an #ifdef sun, which is not correct.
>
> If the modified patch looks ok to you, I will commit it.
>
> -Mark
>
>>
>> Any comments are appreciated.
>> Thanks.
>>
>> Cheers,
>> Mike
>>
>>
>> --
>> Cheers,
>> Mike
>
>> Index: sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
>> ===================================================================
>> --- sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c   (revision 257743)
>> +++ sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c   (working copy)
>> @@ -211,7 +211,7 @@
>>  static vmem_t                *dtrace_minor;          /* minor number arena */
>>  #else
>>  static taskq_t               *dtrace_taskq;          /* task queue */
>> -static struct unrhdr *dtrace_arena;          /* Probe ID number.     */
>> +static vmem_t                *dtrace_arena;          /* probe ID arena */
>>  #endif
>>  static dtrace_probe_t        **dtrace_probes;        /* array of all probes */
>>  static int           dtrace_nprobes;         /* number of probes */
>> @@ -7829,11 +7829,7 @@
>>               kmem_free(probe->dtpr_mod, strlen(probe->dtpr_mod) + 1);
>>               kmem_free(probe->dtpr_func, strlen(probe->dtpr_func) + 1);
>>               kmem_free(probe->dtpr_name, strlen(probe->dtpr_name) + 1);
>> -#if defined(sun)
>> -             vmem_free(dtrace_arena, (void *)(uintptr_t)(probe->dtpr_id), 1);
>> -#else
>> -             free_unr(dtrace_arena, probe->dtpr_id);
>> -#endif
>> +             vmem_free(dtrace_arena, (vmem_addr_t)(probe->dtpr_id), 1);
>>               kmem_free(probe, sizeof (dtrace_probe_t));
>>       }
>>
>> @@ -7950,11 +7946,7 @@
>>               kmem_free(probe->dtpr_func, strlen(probe->dtpr_func) + 1);
>>               kmem_free(probe->dtpr_name, strlen(probe->dtpr_name) + 1);
>>               kmem_free(probe, sizeof (dtrace_probe_t));
>> -#if defined(sun)
>> -             vmem_free(dtrace_arena, (void *)((uintptr_t)i + 1), 1);
>> -#else
>> -             free_unr(dtrace_arena, i + 1);
>> -#endif
>> +             vmem_free(dtrace_arena, (vmem_addr_t)i + 1, 1);
>>       }
>>
>>       mutex_exit(&dtrace_lock);
>> @@ -7990,12 +7982,8 @@
>>               mutex_enter(&dtrace_lock);
>>       }
>>
>> -#if defined(sun)
>> -     id = (dtrace_id_t)(uintptr_t)vmem_alloc(dtrace_arena, 1,
>> -         VM_BESTFIT | VM_SLEEP);
>> -#else
>> -     id = alloc_unr(dtrace_arena);
>> -#endif
>> +     id = (dtrace_id_t)vmem_alloc(dtrace_arena, 1,
>> +                                                              M_BESTFIT | M_WAITOK, NULL);
>>       probe = kmem_zalloc(sizeof (dtrace_probe_t), KM_SLEEP);
>>
>>       probe->dtpr_id = id;
>> @@ -10184,7 +10172,8 @@
>>       aggid = (dtrace_aggid_t)(uintptr_t)vmem_alloc(state->dts_aggid_arena, 1,
>>           VM_BESTFIT | VM_SLEEP);
>>  #else
>> -     aggid = alloc_unr(state->dts_aggid_arena);
>> +     aggid = (dtrace_aggid_t)(uintptr_t)vmem_alloc(state->dts_aggid_arena, 1,
>> +                                                                                               M_BESTFIT | M_WAITOK, NULL);
>>  #endif
>>
>>       if (aggid - 1 >= state->dts_naggregations) {
>> @@ -10237,7 +10226,7 @@
>>  #if defined(sun)
>>       vmem_free(state->dts_aggid_arena, (void *)(uintptr_t)aggid, 1);
>>  #else
>> -     free_unr(state->dts_aggid_arena, aggid);
>> +     vmem_free(state->dts_aggid_arena, (vmem_addr_t)aggid, 1);
>>  #endif
>>
>>       ASSERT(state->dts_aggregations[aggid - 1] == agg);
>> @@ -13211,10 +13200,10 @@
>>       state = kmem_zalloc(sizeof(dtrace_state_t), KM_SLEEP);
>>  #endif
>>
>> +#if defined(sun)
>>       state->dts_epid = DTRACE_EPIDNONE + 1;
>>
>>       (void) snprintf(c, sizeof (c), "dtrace_aggid_%d", m);
>> -#if defined(sun)
>>       state->dts_aggid_arena = vmem_create(c, (void *)1, UINT32_MAX, 1,
>>           NULL, NULL, NULL, 0, VM_SLEEP | VMC_IDENTIFIER);
>>
>> @@ -13229,9 +13218,10 @@
>>       if (devp != NULL)
>>               *devp = state->dts_dev;
>>  #else
>> -     state->dts_aggid_arena = new_unrhdr(1, INT_MAX, &dtrace_unr_mtx);
>> +     state->dts_aggid_arena = vmem_create(c, (vmem_addr_t)1, UINT32_MAX, 1,
>> +         0, M_WAITOK);
>>       state->dts_dev = dev;
>> -#endif
>> +#endif
>>
>>       /*
>>        * We allocate NCPU buffers.  On the one hand, this can be quite
>> @@ -14036,11 +14026,7 @@
>>       dtrace_format_destroy(state);
>>
>>       if (state->dts_aggid_arena != NULL) {
>> -#if defined(sun)
>>               vmem_destroy(state->dts_aggid_arena);
>> -#else
>> -             delete_unrhdr(state->dts_aggid_arena);
>> -#endif
>>               state->dts_aggid_arena = NULL;
>>       }
>>  #if defined(sun)
>> @@ -15375,7 +15361,7 @@
>>  #if defined(sun)
>>               vmem_free(dtrace_arena, (void *)(uintptr_t)probe->dtpr_id, 1);
>>  #else
>> -             free_unr(dtrace_arena, probe->dtpr_id);
>> +             vmem_free(dtrace_arena, (vmem_addr_t)probe->dtpr_id, 1);
>>  #endif
>>               kmem_free(probe, sizeof (dtrace_probe_t));
>>       }
>> Index: sys/cddl/contrib/opensolaris/uts/common/sys/dtrace_impl.h
>> ===================================================================
>> --- sys/cddl/contrib/opensolaris/uts/common/sys/dtrace_impl.h (revision 257743)
>> +++ sys/cddl/contrib/opensolaris/uts/common/sys/dtrace_impl.h (working copy)
>> @@ -1139,11 +1139,7 @@
>>       int dts_nspeculations;                  /* number of speculations */
>>       int dts_naggregations;                  /* number of aggregations */
>>       dtrace_aggregation_t **dts_aggregations; /* aggregation array */
>> -#if defined(sun)
>>       vmem_t *dts_aggid_arena;                /* arena for aggregation IDs */
>> -#else
>> -     struct unrhdr *dts_aggid_arena;         /* arena for aggregation IDs */
>> -#endif
>>       uint64_t dts_errors;                    /* total number of errors */
>>       uint32_t dts_speculations_busy;         /* number of spec. busy */
>>       uint32_t dts_speculations_unavail;      /* number of spec unavail */
>> Index: sys/cddl/dev/dtrace/dtrace_load.c
>> ===================================================================
>> --- sys/cddl/dev/dtrace/dtrace_load.c (revision 257743)
>> +++ sys/cddl/dev/dtrace/dtrace_load.c (working copy)
>> @@ -84,9 +84,9 @@
>>       mutex_enter(&cpu_lock);
>>
>>       ASSERT(MUTEX_HELD(&cpu_lock));
>> +     dtrace_arena = vmem_create("dtrace", (vmem_addr_t)1,
>> +                                                        UINT32_MAX, 1, 0, M_WAITOK | M_BESTFIT);
>>
>> -     dtrace_arena = new_unrhdr(1, INT_MAX, &dtrace_unr_mtx);
>> -
>>       dtrace_state_cache = kmem_cache_create("dtrace_state_cache",
>>           sizeof (dtrace_dstate_percpu_t) * NCPU, DTRACE_STATE_ALIGN,
>>           NULL, NULL, NULL, NULL, NULL, 0);
>> Index: sys/cddl/dev/dtrace/dtrace_unload.c
>> ===================================================================
>> --- sys/cddl/dev/dtrace/dtrace_unload.c       (revision 257743)
>> +++ sys/cddl/dev/dtrace/dtrace_unload.c       (working copy)
>> @@ -104,7 +104,7 @@
>>
>>       kmem_cache_destroy(dtrace_state_cache);
>>
>> -     delete_unrhdr(dtrace_arena);
>> +     vmem_destroy(dtrace_arena);
>>
>>       if (dtrace_toxrange != NULL) {
>>               kmem_free(dtrace_toxrange, 0);
>
>> _______________________________________________
>> freebsd-dtrace@freebsd.org mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-dtrace
>> To unsubscribe, send any mail to "freebsd-dtrace-unsubscribe@freebsd.org"
>



-- 
Cheers,
Mike



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CA%2BcqwfdBKY7Y9t3RyLtQNAYwiDT=PPD0dyeoe4Uk-iXUdTk3dg>