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 | +------------------+--------------------------+------------------------+
