Re: Another kvm user can go away?

[Paul Goyette <paul%whooppee.com@localhost>]

> What function does kern_history_new serve?

It should be tested in sysctl_kernhist_helper() to decide whether or
not it is necessary to call sysctl_hist_new().

However, I'm not entirely convinced it is really necessary.  In only
allows us to avoid a walk of a very short LIST (typically only a
handful of entries, with max of 32 entries).


> Why membar_producer?  No matching membar_(datadep_)consumer?

If I retain the kern_history_new flag (see above), there should be a
membar_consumer() before testing it in sysctl_kernhist_helper().


> Is sysctl_createv kosher under a mutex?  I expect it may allocate
> memory, so my inclination is probably not.

Hmmm.  I would hope the sysctl_createv() used KM_SLEEP for any memory
allocation it might do.  And sysctl_craetev() itself obtains and
releases its own lock.  A quick search of all sys/kern/*sysc*.c files
shows no attempts to call kmem_alloc(..., KM_NOSLEEP) so this should
be safe.

I'm pretty sure I could remove this mutex, with few ill effects.  If
there were two concurrent attempts to create the same new kernhist
sysctl node, they would both lock internally in sysctl_createv() and
proceed sequentially.  One caller would complete normally, and the
other caller would end up creating a duplicate node which would be
ignored (that caller would get a pointer to the original node.)

So, yeah, I think I'll just remove the mutex!  :)


> Can you sketch the control and data flow here?  sysctl_kernhist_helper
> is the function of a new sysctl node kern.hist, but it itself
> establishes new sysctl nodes, which is a curious kind of recursion.

sysctl_kernhist_helper() is really fairly simple:

1. Make sure that any newly-created histories have a sysctl node.  I
   added a new field to the history header to contain the sysctl node
   number, to determine if the sysctl node is already created.

2. If it's a CTL_QUERY, just pass it along.  Otherwise make some
   sanity checks.

3. Figure out which kernhist is associated with this sysctl node.

4. Allocate a translation table (to track correlation between string
   addresses in kernel space vs offsets into the string portion of
   the export structure.  Then examine each history record to find
   all unique strings, and enter them in the table.  Once we're done
   with this pass, we know how much space the string area will need.

5. Allocate the export structure, including the proper amount of
   string space.  Then translate each history record into the export
   structure, replacing string pointers by "offsets into the string
   space".

6. Finally, loop through the string "translation" table, and copy
   the strings into the export structure.

7. copyout() the data.  If the output buffer was not large enough,
   and no other error occurred from copyout, report ENOMEM.  In
   any case, report the amount of data that we attempted to copy.

8. kmem_free() the temporary structures.


> There seem to be some stray debug prints in sysctl_kernhist_helper.

Ooops, I thought I got rid of those!  Thanks!



Thanks for the detailed review.



+------------------+--------------------------+------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:      |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+