[DTrace-devel] [kris.van.hees at oracle.com: Re: [PATCH 3/7] Add rodata support]
Kris Van Hees
kris.van.hees at oracle.com
Thu Aug 3 16:48:34 UTC 2023
Accidentally sent my reply as a direct email - fwd'ing to the list.
----- Forwarded message from Kris Van Hees <kris.van.hees at oracle.com> -----
Date: Thu, 3 Aug 2023 12:40:49 -0400
From: Kris Van Hees <kris.van.hees at oracle.com>
To: Eugene Loh <eugene.loh at oracle.com>
Subject: Re: [DTrace-devel] [PATCH 3/7] Add rodata support
On Thu, Jul 27, 2023 at 04:02:22PM -0400, Eugene Loh wrote:
> This is a kind of long patch, and I do not know what problem it is
> trying to solve.
Well, it provides for read-only data to be available to BPF programs. Right
now, we only have strtab as read-only data, and the zero area. But there is
a need to have more arbitrary read-only data as well. The first user for
that will be inet_ntoa6() but future BPF programs can make use of this as
well to support read-only data (arrays, structs, and strings not subject to
the strsize limit).
> If there is supposed to be read-only data, wouldn't that include strtab
> and the zeroes?? Why isn't this mechanism being used for them?? Or, why
> not skip this stuff entirely and just add the inet_ntoa6 table (when it
> appears down the road) in the same way that the zeroes were added?? As
> it is, we simply seem to be adding a lot of complexity (and duplicating
> a lot of code from strtab) for no clear purpose.
I considered just adding the inet_ntoa6 table but that seems like a very
specialized things to add. The next read-only data item that pops up then
requires us to do more specialized code, or to then make things more generic.
I'd rather just go with generic right now.
And yes, the string table can be implemented as a special version of rodata,
as can the zero area be added as an item in the rodata. That is future
optimization I plan to do - consolidating things.
> A big comment block in dt_rodata.h or .c explaining this stuff would be
> good.
I disagree - I don't think source code files need lengthy justification.
> Meanwhile other comments below.
>
> On 7/27/23 11:29, Kris Van Hees via DTrace-devel wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> > libdtrace/Build | 1 +
> > libdtrace/dt_bpf.c | 9 +-
> > libdtrace/dt_bpf.h | 38 +++---
> > libdtrace/dt_cc.c | 6 +
> > libdtrace/dt_cg.c | 11 ++
> > libdtrace/dt_dctx.h | 2 +
> > libdtrace/dt_dlibs.c | 2 +
> > libdtrace/dt_impl.h | 10 +-
> > libdtrace/dt_open.c | 1 +
> > libdtrace/dt_rodata.c | 295 ++++++++++++++++++++++++++++++++++++++++++
> > libdtrace/dt_rodata.h | 35 +++++
> > libdtrace/dt_subr.c | 22 ++++
> > 12 files changed, 411 insertions(+), 21 deletions(-)
> > create mode 100644 libdtrace/dt_rodata.c
> > create mode 100644 libdtrace/dt_rodata.h
> >
> > diff --git a/libdtrace/Build b/libdtrace/Build
> > index a392c5f3..d1b00933 100644
> > --- a/libdtrace/Build
> > +++ b/libdtrace/Build
> > @@ -61,6 +61,7 @@ libdtrace-build_SOURCES = dt_aggregate.c \
> > dt_provider_sdt.c \
> > dt_provider_tp.c \
> > dt_regset.c \
> > + dt_rodata.c \
> > dt_string.c \
> > dt_strtab.c \
> > dt_subr.c \
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index 54086077..b4c880bc 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -711,7 +711,9 @@ gmap_create_strtab(dtrace_hdl_t *dtp)
>
> The big block comment describing gmap_create_strtab() should acknowledge
> the addition of rodata.
Good point.
> > int fd, rc, err;
> >
> > dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
> > - dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_strlen, 8);
> > + dtp->dt_rooffset = P2ROUNDUP(dtp->dt_strlen, 8);
> > + dtp->dt_rosize = dt_rodata_size(dtp->dt_rodata);
> > + dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_rooffset + dtp->dt_rosize, 8);
> >
> > /*
> > * Ensure the zero-filled memory at the end of the strtab is large
> > @@ -730,6 +732,7 @@ gmap_create_strtab(dtrace_hdl_t *dtp)
> > if (strtab == NULL)
> > return dt_set_errno(dtp, EDT_NOMEM);
> >
> > + /* Copy the string table data. */
> > dt_strtab_write(dtp->dt_ccstab, (dt_strtab_write_f *)dt_strtab_copystr,
> > strtab);
>
> The new comment is helpful.? So a few statements earlier, there should
> also be a comment about the allocation.
Well, allocation is rather obvious. Here I am pointing out that we are writing
the string table data (because we are going to be writing rodata to this map
also, a bit further down).
> > @@ -745,6 +748,10 @@ gmap_create_strtab(dtrace_hdl_t *dtp)
> > buf += len + 1;
> > }
> >
> > + /* Copy the read-only data. */
> > + dt_rodata_write(dtp->dt_rodata, (dt_rodata_copy_f *)dt_rodata_copy,
> > + strtab + dtp->dt_rooffset);
> > +
> > fd = create_gmap(dtp, "strtab", BPF_MAP_TYPE_ARRAY, sizeof(uint32_t),
> > sz, 1);
> > if (fd == -1)
>
> I do not see where this is going.? Why are we passing dt_rodata_copy to
> a function that is in a file that knows more about dt_rodata_copy than
> we do?? Is some future alternative use anticipated?? If so, how about
> allowing someone to pass a NULL function meaning "you know what to do."?
> I suppose the real issue is we're trying to mimic strtab.
I kept the way strtab handles this because I plan to convert the strtab code
into a variant of rodata. Keeping this callback function the same makes that
a lot easier.
> Anyhow, how about adding another comment before the create_gmap() call?
Why would we add a comment before the create_gmap() call? That call is
rather obvious, is it not?
> > diff --git a/libdtrace/dt_bpf.h b/libdtrace/dt_bpf.h
> > index f414e78e..e64ca3c1 100644
> > --- a/libdtrace/dt_bpf.h
> > +++ b/libdtrace/dt_bpf.h
> > @@ -19,25 +19,27 @@ struct dtrace_hdl;
> > extern "C" {
> > #endif
> >
> > -#define DT_CONST_EPID 1
> > -#define DT_CONST_PRID 2
> > -#define DT_CONST_CLID 3
> > -#define DT_CONST_ARGC 4
> > -#define DT_CONST_STBSZ 5
> > -#define DT_CONST_STRSZ 6
> > -#define DT_CONST_STKSIZ 7
> > -#define DT_CONST_BOOTTM 8
> > -#define DT_CONST_NSPEC 9
> > -#define DT_CONST_NCPUS 10
> > -#define DT_CONST_PC 11
> > -#define DT_CONST_TUPSZ 12
> > -#define DT_CONST_TASK_PID 13
> > -#define DT_CONST_TASK_TGID 14
> > +#define DT_CONST_EPID 1
> > +#define DT_CONST_PRID 2
> > +#define DT_CONST_CLID 3
> > +#define DT_CONST_ARGC 4
> > +#define DT_CONST_STBSZ 5
> > +#define DT_CONST_STRSZ 6
> > +#define DT_CONST_STKSIZ 7
> > +#define DT_CONST_BOOTTM 8
> > +#define DT_CONST_NSPEC 9
> > +#define DT_CONST_NCPUS 10
> > +#define DT_CONST_PC 11
> > +#define DT_CONST_TUPSZ 12
> > +#define DT_CONST_TASK_PID 13
> > +#define DT_CONST_TASK_TGID 14
> > #define DT_CONST_TASK_REAL_PARENT 15
> > -#define DT_CONST_TASK_COMM 16
> > -#define DT_CONST_MUTEX_OWNER 17
> > -#define DT_CONST_RWLOCK_CNTS 18
> > -#define DT_CONST_ZERO_OFF 19
> > +#define DT_CONST_TASK_COMM 16
> > +#define DT_CONST_MUTEX_OWNER 17
> > +#define DT_CONST_RWLOCK_CNTS 18
> > +#define DT_CONST_RODATA_OFF 19
> > +#define DT_CONST_RODATA_SIZE 20
> > +#define DT_CONST_ZERO_OFF 21
> >
> > #define DT_BPF_LOG_SIZE_DEFAULT (UINT32_MAX >> 8)
> > #define DT_BPF_LOG_SIZE_SMALL 4096
> > diff --git a/libdtrace/dt_cc.c b/libdtrace/dt_cc.c
> > index 88d7f1fe..2293892c 100644
> > --- a/libdtrace/dt_cc.c
> > +++ b/libdtrace/dt_cc.c
> > @@ -2468,6 +2468,12 @@ dt_link_construct(dtrace_hdl_t *dtp, const dt_probe_t *prp, dtrace_difo_t *dp,
> > nrp->dofr_data = total_offset;
> > continue;
> > }
> > + case DT_CONST_RODATA_OFF:
> > + nrp->dofr_data = dtp->dt_rooffset;
> > + continue;
> > + case DT_CONST_RODATA_SIZE:
> > + nrp->dofr_data = dtp->dt_rosize;
> > + continue;
> > case DT_CONST_ZERO_OFF:
> > nrp->dofr_data = dtp->dt_zerooffset;
> > continue;
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index bb6cff4a..6a1413f0 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -56,6 +56,7 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > dt_ident_t *mem = dt_dlib_get_map(dtp, "mem");
> > dt_ident_t *state = dt_dlib_get_map(dtp, "state");
> > dt_ident_t *prid = dt_dlib_get_var(pcb->pcb_hdl, "PRID");
>
> While we're here, how about s/pcb->pcb_hdl/dtp/?
Sure.
> > + dt_ident_t *ro_off = dt_dlib_get_var(dtp, "RODATA_OFF");
> > uint_t lbl_exit = pcb->pcb_exitlbl;
> >
> > assert(aggs != NULL);
> > @@ -206,6 +207,16 @@ dt_cg_tramp_prologue_act(dt_pcb_t *pcb, dt_activity_t act)
> > } while(0)
> >
> > DT_CG_STORE_MAP_PTR("strtab", DCTX_STRTAB);
> > +
> > + /*
> > + * We know that a pointer to the strtab data is in %r0 (because that is
> > + * where the DT_CG_STORE_MAP_PTR() macro left it, and we know that dctx
> > + * is in %r9. So, we can just add RODATA_OFF to %r0, and store that in
> > + * [%r9 + DCTX_RODATA].
> > + */
> > + emite(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, -1), ro_off);
> > + emit(dlp, BPF_STORE(BPF_DW, BPF_REG_9, DCTX_RODATA, BPF_REG_0));
> > +
> > if (dtp->dt_options[DTRACEOPT_SCRATCHSIZE] > 0)
> > DT_CG_STORE_MAP_PTR("scratchmem", DCTX_SCRATCHMEM);
> > if (dt_idhash_datasize(dtp->dt_globals) > 0)
> > diff --git a/libdtrace/dt_dctx.h b/libdtrace/dt_dctx.h
> > index 315bdc37..86bfe6f2 100644
> > --- a/libdtrace/dt_dctx.h
> > +++ b/libdtrace/dt_dctx.h
> > @@ -58,6 +58,7 @@ typedef struct dt_dctx {
> > char *mem; /* General scratch memory */
> > char *scratchmem; /* Scratch space for alloca, etc */
> > char *strtab; /* String constant table */
> > + char *rodata; /* Read-only data */
> > char *agg; /* Aggregation data */
> > char *gvars; /* Global variables */
> > char *lvars; /* Local variables */
> > @@ -70,6 +71,7 @@ typedef struct dt_dctx {
> > #define DCTX_MEM offsetof(dt_dctx_t, mem)
> > #define DCTX_SCRATCHMEM offsetof(dt_dctx_t, scratchmem)
> > #define DCTX_STRTAB offsetof(dt_dctx_t, strtab)
> > +#define DCTX_RODATA offsetof(dt_dctx_t, rodata)
> > #define DCTX_AGG offsetof(dt_dctx_t, agg)
> > #define DCTX_GVARS offsetof(dt_dctx_t, gvars)
> > #define DCTX_LVARS offsetof(dt_dctx_t, lvars)
> > diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> > index 1005e027..02a24ad2 100644
> > --- a/libdtrace/dt_dlibs.c
> > +++ b/libdtrace/dt_dlibs.c
> > @@ -91,6 +91,8 @@ static const dt_ident_t dt_bpf_symbols[] = {
> > DT_BPF_SYMBOL_ID(TASK_COMM, DT_IDENT_SCALAR, DT_CONST_TASK_COMM),
> > DT_BPF_SYMBOL_ID(MUTEX_OWNER, DT_IDENT_SCALAR, DT_CONST_MUTEX_OWNER),
> > DT_BPF_SYMBOL_ID(RWLOCK_CNTS, DT_IDENT_SCALAR, DT_CONST_RWLOCK_CNTS),
> > + DT_BPF_SYMBOL_ID(RODATA_OFF, DT_IDENT_SCALAR, DT_CONST_RODATA_OFF),
> > + DT_BPF_SYMBOL_ID(RODATA_SIZE, DT_IDENT_SCALAR, DT_CONST_RODATA_SIZE),
> > DT_BPF_SYMBOL_ID(ZERO_OFF, DT_IDENT_SCALAR, DT_CONST_ZERO_OFF),
> >
> > /* End-of-list marker */
> > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > index 42a7ccaa..adc8d8db 100644
> > --- a/libdtrace/dt_impl.h
> > +++ b/libdtrace/dt_impl.h
> > @@ -34,6 +34,7 @@ extern "C" {
> > #include <dt_bpf_maps.h>
> > #include <dt_parser.h>
> > #include <dt_regset.h>
> > +#include <dt_rodata.h>
> > #include <dt_strtab.h>
> > #include <dt_symtab.h>
> > #include <dt_ident.h>
> > @@ -306,14 +307,17 @@ struct dtrace_hdl {
> > dt_idhash_t *dt_tls; /* hash table of thread-local identifiers */
> > dt_idhash_t *dt_bpfsyms;/* hash table of BPF identifiers */
> > dt_strtab_t *dt_ccstab; /* global string table (during compilation) */
> > + dt_rodata_t *dt_rodata; /* global read-only data */
> > uint_t dt_strlen; /* global string table (runtime) size */
> > + uint_t dt_rooffset; /* read-only data offset */
> > + uint_t dt_rosize; /* read-only data size */
> > + uint_t dt_zerooffset; /* zero region, offset */
> > + uint_t dt_zerosize; /* zero region, size */
> > uint_t dt_maxreclen; /* largest record size across programs */
> > uint_t dt_maxdvarsize; /* largest dynamic variable across programs */
> > uint_t dt_maxtuplesize; /* largest tuple across programs */
> > uint_t dt_maxlvaralloc; /* largest lvar alloc across pcbs */
> > uint_t dt_maxaggdsize; /* largest aggregation data sizw */
> > - uint_t dt_zerosize; /* zero region, size */
> > - uint_t dt_zerooffset; /* zero region, offset */
> > dt_tstring_t *dt_tstrings; /* temporary string slots */
> > dt_list_t dt_modlist; /* linked list of dt_module_t's */
> > dt_htab_t *dt_mods; /* hash table of dt_module_t's */
> > @@ -725,6 +729,8 @@ extern int dt_version_defined(dt_version_t);
> >
> > extern int dt_str2kver(const char *, dt_version_t *);
> >
> > +extern uint32_t dt_gen_hval(const char *, uint32_t, size_t);
> > +
> > /*
> > * Miscellaneous internal libdtrace interfaces. The definitions below are for
> > * libdtrace routines that do not yet merit their own separate header file.
> > diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> > index b6a0b623..a90601b0 100644
> > --- a/libdtrace/dt_open.c
> > +++ b/libdtrace/dt_open.c
> > @@ -888,6 +888,7 @@ dt_vopen(int version, int flags, int *errp,
> > DIF_VAR_OTHER_UBASE, DIF_VAR_OTHER_MAX);
> >
> > dtp->dt_ccstab = dt_strtab_create(BUFSIZ);
> > + dtp->dt_rodata = dt_rodata_create(BUFSIZ);
>
> Memory leak?? No companion destroy call?? (No big deal.)
>
> And do we need to check if NULL was returned?
Yes, thanks.
>
> > dtp->dt_strlen = 0;
> >
> > if (dtp->dt_macros == NULL || dtp->dt_aggs == NULL ||
> > diff --git a/libdtrace/dt_rodata.c b/libdtrace/dt_rodata.c
> > new file mode 100644
> > index 00000000..cba12d77
> > --- /dev/null
> > +++ b/libdtrace/dt_rodata.c
> > @@ -0,0 +1,295 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
>
> Why 2006?? Because we're duplicating code from strtab?
Forgot to update - will do.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <assert.h>
> > +
> > +#include <dt_rodata.h>
> > +#include <dt_string.h>
> > +#include <dt_impl.h>
> > +
> > +typedef struct dt_rohash dt_rohash_t;
> > +struct dt_rohash {
> > + const char *data; /* pointer to actual data */
> > + size_t buf; /* index of data buffer */
> > + size_t off; /* offset in bytes of this item */
> > + size_t len; /* length in bytes of this item */
> > + dt_rohash_t *next; /* next item in hash chain */
> > +};
> > +
> > +struct dt_rodata {
> > + dt_rohash_t **hash; /* array of hash buckets */
> > + size_t hashsz; /* size of hash bucket array */
> > + char **bufs; /* array of buffer pointers */
> > + char *ptr; /* pointer to current buffer location */
> > + size_t nbufs; /* size of buffer pointer array */
> > + size_t bufsz; /* size of individual buffer */
> > + size_t count; /* total number of items in rodata */
> > + size_t size; /* total size of items in bytes */
> > +};
> > +
> > +static int
> > +dt_rodata_grow(dt_rodata_t *dp)
> > +{
> > + char *ptr, **bufs;
> > +
> > + ptr = malloc(dp->bufsz);
> > + if (ptr == NULL)
> > + return -1;
> > +
> > + bufs = realloc(dp->bufs, (dp->nbufs + 1) * sizeof(char *));
> > + if (bufs == NULL) {
> > + free(ptr);
> > + return -1;
> > + }
> > +
> > + dp->nbufs++;
> > + dp->bufs = bufs;
> > + dp->ptr = ptr;
> > + dp->bufs[dp->nbufs - 1] = dp->ptr;
>
> No big deal, but could "= ptr" here.
>
> > +
> > + return 0;
> > +}
> > +
> > +dt_rodata_t *
> > +dt_rodata_create(size_t bufsz)
> > +{
> > + dt_rodata_t *dp = malloc(sizeof(dt_rodata_t));
> > + size_t nbuckets = _dtrace_strbuckets;
> > +
> > + assert(bufsz != 0);
> > +
> > + if (dp == NULL)
> > + return NULL;
> > +
> > + memset(dp, 0, sizeof(dt_rodata_t));
> > + dp->hash = malloc(nbuckets * sizeof(dt_rohash_t *));
> > + if (dp->hash == NULL)
> > + goto err;
> > +
> > + memset(dp->hash, 0, nbuckets * sizeof(dt_rohash_t *));
> > + dp->hashsz = nbuckets;
> > + dp->bufs = NULL;
> > + dp->ptr = NULL;
> > + dp->nbufs = 0;
> > + dp->bufsz = bufsz;
> > +
> > + if (dt_rodata_grow(dp) == -1)
> > + goto err;
> > +
> > + /* Pre-populate the read-only data with a 0-byte at offset 0. */
>
> Don't hyphenate 0-byte?
>
> > + *dp->ptr++ = 0;
> > + dp->size = 1;
> > + dp->count = 1;
> > +
> > + return dp;
> > +
> > +err:
> > + dt_rodata_destroy(dp);
> > + return NULL;
> > +}
> > +
> > +void
> > +dt_rodata_destroy(dt_rodata_t *dp)
> > +{
> > + dt_rohash_t *hp, *hq;
> > + size_t i;
> > +
> > + if (dp == NULL)
> > + return;
> > +
> > + for (i = 0; i < dp->hashsz; i++) {
> > + for (hp = dp->hash[i]; hp != NULL; hp = hq) {
> > + hq = hp->next;
> > + free(hp);
> > + }
> > + }
> > +
> > + for (i = 0; i < dp->nbufs; i++)
> > + free(dp->bufs[i]);
> > +
> > + if (dp->hash != NULL)
> > + free(dp->hash);
> > + if (dp->bufs != NULL)
> > + free(dp->bufs);
>
> If dp->hash or dp->bufs could be NULL, we should have checked earlier,
> before using them.
dp->hash cannot be NULL if dp->hashsz > 0, and likewise for dp->bufs.
> > +
> > + free(dp);
> > +}
> > +
> > +static int
> > +dt_rodata_compare(dt_rodata_t *dp, dt_rohash_t *hp, const char *ptr, size_t len)
> > +{
> > + size_t b = hp->buf;
> > + const char *buf = hp->data;
> > + size_t resid, n;
> > + int rv;
> > +
> > + while (len != 0) {
> > + if (buf == dp->bufs[b] + dp->bufsz)
> > + buf = dp->bufs[++b];
> > +
> > + resid = dp->bufs[b] + dp->bufsz - buf;
> > + n = MIN(resid, len);
> > +
> > + if ((rv = memcmp(buf, ptr, n)) != 0)
> > + return rv;
> > +
> > + buf += n;
> > + ptr += n;
> > + len -= n;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +dt_rodata_copyin(dt_rodata_t *dp, const char *ptr, size_t len)
> > +{
> > + char *old_p = dp->ptr;
> > + size_t old_n = dp->nbufs;
> > + size_t b = dp->nbufs - 1;
> > + size_t resid, n;
> > +
> > + while (len != 0) {
> > + if (dp->ptr == dp->bufs[b] + dp->bufsz) {
> > + if (dt_rodata_grow(dp) == -1)
> > + goto err;
> > + b++;
> > + }
> > +
> > + resid = dp->bufs[b] + dp->bufsz - dp->ptr;
> > + n = MIN(resid, len);
> > + memcpy(dp->ptr, ptr, n);
> > +
> > + dp->ptr += n;
> > + ptr += n;
> > + len -= n;
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + while (dp->nbufs != old_n)
> > + free(dp->bufs[--dp->nbufs]);
> > +
> > + dp->ptr = old_p;
> > + return -1;
> > +}
> > +
> > +static ssize_t
> > +dt_rodata_xindex(dt_rodata_t *dp, const char *ptr, size_t len, uint32_t h)
>
> How about putting the h%dp->hashsz in here to ensure it's done?
> In fact, just move the h=dt_gen_hval() in here rather than passing h in.
>
> > +{
> > + dt_rohash_t *hp;
> > +
> > + for (hp = dp->hash[h]; hp != NULL; hp = hp->next) {
>
> No need for these brackets?
>
> > + if (dt_rodata_compare(dp, hp, ptr, len) == 0)
> > + return hp->off;
> > + }
> > +
> > + return -1;
> > +}
> > +
> > +ssize_t
> > +dt_rodata_index(dt_rodata_t *dp, const char *ptr, size_t len)
>
> No one uses this?
> Or is dt_rodata_insert() supposed to call it?
> In which case, xindex can be inlined here.
>
> > +{
> > + ssize_t rc;
> > + uint32_t h;
> > +
> > + if (ptr == NULL || ptr[0] == '\0')
> > + return 0; /* NULL or a single 0-byte are at offset 0. */
> > +
> > + h = dt_gen_hval(ptr, len, len) % dp->hashsz;
>
> Why len, len?? Why not 0, len?
Because using len as initial seed for the hash function works well.
> > + rc = dt_rodata_xindex(dp, ptr, len, h);
> > +
> > + return rc;
> > +}
> > +
> > +ssize_t
> > +dt_rodata_insert(dt_rodata_t *dp, const char *ptr, size_t len)
> > +{
> > + dt_rohash_t *hp;
> > + ssize_t off;
> > + uint32_t h;
> > +
> > + if (ptr == NULL || ptr[0] == '\0')
> > + return 0; /* NULL or a single 0-byte are at offset 0. */
> > +
> > + h = dt_gen_hval(ptr, len, len) % dp->hashsz;
>
> Ditto.
>
> > + off = dt_rodata_xindex(dp, ptr, len, h);
> > + if (off != -1)
> > + return off;
> > +
> > + /*
> > + * Create a new hash bucket, initialize it, and insert it at the front
> > + * of the hash chain for the appropriate bucket.
> > + */
> > + if ((hp = malloc(sizeof(dt_rohash_t))) == NULL)
> > + return -1L;
> > +
> > + hp->data = dp->ptr;
> > + hp->buf = dp->nbufs - 1;
> > + hp->off = dp->size;
> > + hp->len = len;
> > + hp->next = dp->hash[h];
> > +
> > + /*
> > + * Now copy the data into our buffer list, and then update the global
> > + * counts of items and bytes. Return the item's byte offset.
> > + */
> > + if (dt_rodata_copyin(dp, ptr, len) == -1) {
> > + free(hp);
> > + return -1L;
> > + }
> > +
> > + dp->count++;
> > + dp->size += len;
> > + dp->hash[h] = hp;
> > +
> > + return hp->off;
> > +}
> > +
> > +size_t
> > +dt_rodata_size(const dt_rodata_t *dp)
> > +{
> > + return dp->size;
> > +}
> > +
> > +/*
> > + * Copy data from the compile-time read-only data into a DIFO-style read-only
> > + * storage memory block.
> > + */
> > +ssize_t
> > +dt_rodata_copy(const char *s, size_t n, size_t off, char *buf)
> > +{
> > + memcpy(buf + off, s, n);
> > + return n;
> > +}
> > +
> > +ssize_t
> > +dt_rodata_write(const dt_rodata_t *dp, dt_rodata_copy_f *func, void *private)
> > +{
> > + ssize_t res, total = 0;
> > + size_t i;
> > + size_t n;
> > +
> > + for (i = 0; i < dp->nbufs; i++, total += res) {
> > + if (i == dp->nbufs - 1)
> > + n = dp->ptr - dp->bufs[i];
> > + else
> > + n = dp->bufsz;
> > +
> > + if ((res = func(dp->bufs[i], n, total, private)) <= 0)
> > + break;
> > + }
> > +
> > + if (total == 0 && dp->size != 0)
> > + return -1;
> > +
> > + return total;
> > +}
> > diff --git a/libdtrace/dt_rodata.h b/libdtrace/dt_rodata.h
> > new file mode 100644
> > index 00000000..d995d7ea
> > --- /dev/null
> > +++ b/libdtrace/dt_rodata.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
>
> Years?
Indeed.
> > + * Licensed under the Universal Permissive License v 1.0 as shown at
> > + * http://oss.oracle.com/licenses/upl.
> > + */
> > +
> > +#ifndef _DT_RODATA_H
> > +#define _DT_RODATA_H
> > +
> > +#include <sys/types.h>
> > +#include <sys/dtrace_types.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +typedef struct dt_rodata dt_rodata_t;
> > +
> > +typedef ssize_t dt_rodata_copy_f(const char *, size_t, size_t, void *);
> > +
> > +extern dt_rodata_t *dt_rodata_create(size_t);
> > +extern void dt_rodata_destroy(dt_rodata_t *);
> > +extern ssize_t dt_rodata_index(dt_rodata_t *, const char *, size_t);
>
> No one uses _index.
... yet
> > +extern ssize_t dt_rodata_insert(dt_rodata_t *, const char *, size_t);
> > +extern size_t dt_rodata_size(const dt_rodata_t *);
> > +extern ssize_t dt_rodata_copy(const char *, size_t, size_t, char *);
>
> Earlier questions about why this is externed.
>
> > +extern ssize_t dt_rodata_write(const dt_rodata_t *, dt_rodata_copy_f *,
> > + void *);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _DT_RODATA_H */
> > diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
> > index 300c1a8d..6af2e0c8 100644
> > --- a/libdtrace/dt_subr.c
> > +++ b/libdtrace/dt_subr.c
> > @@ -985,3 +985,25 @@ dt_str2kver(const char *kverstr, dt_version_t *vp)
> >
> > return 0;
> > }
> > +
> > +/*
> > + * Compute a 32-bit hash value for a memory block of given size.
> > + */
> > +uint32_t dt_gen_hval(const char *p, uint32_t hval, size_t len)
> > +{
> > + uint32_t g;
> > +
> > + if (!p || len == 0)
> > + return hval;
> > +
> > + while (len--) {
> > + hval = (hval << 4) + *p++;
> > + g = hval & 0xf0000000;
> > + if (g != 0) {
> > + hval ^= (g >> 24);
> > + hval ^= g;
> > + }
> > + }
> > +
> > + return hval;
> > +}
>
> Same algorithm as
> ??? dt_spec_buf_hval()
> ??? str2hval()
> ??? string_hash()
> Can anything be consolidated here?
It will. I first wanted to get rodata in the tree, and inet_ntoa6() out.
Then I will introduce patches to rework existing code to consolidate.
----- End forwarded message -----
More information about the DTrace-devel
mailing list