[DTrace-devel] [PATCH v2 10/20] libproc: add Pinode_to_map()

[Nick Alcock]

On 22 Sep 2022, Kris Van Hees spake thusly:

> On Wed, Sep 07, 2022 at 01:59:58PM +0100, Nick Alcock via DTrace-devel wrote:
>> This lets you hand in a (dev, ino) pair and get back the prmap_file_t
>> entry that corresponds to it.  This is useful when dealing with uprobes,
>> which work on an *inode* basis, not a process basis or even a mapping
>> basis.
>
> I think this could use a bit more explanation.  For one, it fails to mention
> that it actually takes a ps_prochandle, dev, and ino and returns a prmap_file_t
> for that.  I.e. this is functionality that operates within a given process.

Oops true! Sorry, I was heads-down in the implementation and that seemed
obvious, but it really isn't.

> So, I think a better commit description is in order.

Adjusted in the next round.

> Also, the implementation *seems* more extensive than is really needed for this
> patch series, but I assume that is with a view on the future?

It's because some processes are pigs with huge numbers of mappings (many
thousands). Emacs has 3000+ these days. Linearly scanning this stuff is
horrible, so we wanted a hash table, but we don't have a decent additive
hash function for integers. Hence NASAM. (Also, for consistency: name ->
mapping uses a hashtab, so so should other mapping lookup functions.)

>>  struct prmap;
>>  typedef struct prmap_file {
>> -	struct prmap_file *prf_next;	/* next in hash chain */
>> +	struct prmap_file *prf_next;	/* next in filename hash chain */
>> +	struct prmap_file *prf_inum_next; /* next in inode hash chain */
>
> Perhaps rename prf_next to prf_name_next, consistent with prf_inum_next?

Good idea.

>> +/*
>> + * Given a process handle, find a corresponding prmap_file by (dev_t, inode
>> + * number), or NULL if none.
>> + */
>> +static prmap_file_t *Pprmap_file_by_dev_inum(struct ps_prochandle *P,
>> +    dev_t dev, ino_t inum)
>> +{
>> +	uint_t h = dev_inum_hash(dev, inum) % MAP_HASH_BUCKETS;
>> +
>> +	prmap_file_t *prf;
>> +
>> +	for (prf = P->map_inum[h]; prf != NULL; prf = prf->prf_inum_next)
>> +		if (prf->first_segment->pr_inum == inum &&
>> +		    prf->first_segment->pr_dev == dev)
>> +			return prf;
>> +
>> +	return NULL;
>> +}
>
> Why is this not simply called Pinode_to_map()?  Since Pinode_to_map() is the
> sole caller for Pprmap_file_by_dev_inum() *and* since it really just calls this
> function, why not name this Pinode_to_map() and be done?

Mostly because it's very closely analogous to Pprmap_file_by_name(),
which *does* have multiple callers (none of which is Pname_to_map(),
since Pname_to_map() uses lmid names, not filenames).  Maybe we want to
rename both Pprmap's to e.g. Pinode_to_map() and Pfilename_to_map()?

... actually, yeah, that seems much nicer (even if only Pinode_to_map()
is exported right now). Adjusted.


... the naming is very annoying here: we have *_to_map() functions
returning, variously, map_info_t, prmap_t, and prmap_file_t seemingly at
random. As a partial step towards disentangling this, I've renamed all
the prmap_file_t-returning functions to *_to_file_map and adjusted all
their callers. So we now have Pinode_to_file_map and
Pfilename_to_file_map (which used to be called Pprmap_file_by_name).
(The userspace commit that calls Pinode_to_map has been adjusted
accordingly.)

-- 
NULL && (void)