CFT/CFR: NUMA policy branch

[Garrett Cooper]

On Jul 5, 2015, at 19:06, Adrian Chadd <adrian at freebsd.org> wrote:

> Hi,
> 
> I've done another update. kib@ has been beating me with the clue stick a bit.
> 
> https://github.com/freebsd/freebsd/compare/master...erikarn:local/adrian_numa_policy
> 
> * (kib) (numactl.c) fix up sorting of include files
> * (kib) (numactl.c) consistent use of values when calling err()
> * (kib) (numactl.c) consistently wrap lines at 78 characters, don't
> prematurely wrap lines
> * (kib) don't use the old-style BSD licence mentioning "regents", use
> the updated one
> * (kib) (vm_domain.c) don't break out after iterating a few times and
> have the API be unpredictable - so now the API will always succeed in
> reading a vm_policy
> 
> I've tested the policies (first-touch, fixed-domain, round-robin) and
> they all still work as advertised, both on threads and processes.
> 
> I'd appreciate more reviews and some further testing.

Please create a dummy pull request or post the code up on Phabricator.

- Please put some of the items like policy_to_str and str_to_policy in a library.
- Please use that code in the kernel as well for sysctl_vm_default_policy to reduce duplication.
- Please note reasoning for why `options MAXMEMDOM=16` in numa(4).
- Why are checking for `if (p)` before calling PROC_UNLOCK(p) in sys_numa_getaffinity, but not sys_numa_setaffinity?
- sys_numa_setaffinity and sys_numa_getaffinity look similar. Could something be implemented like sysctl(3) for handling getting/setting of affinities all in one shot?
- `if (p)` should be `if (p != NULL)`, etc per style(9).
- Is there a way that the affinity could be inherited/not inherited across threads, similar to what ktrace -i does? If so, how does one do that?
- In vm_domain_rr_selectdomain, should this use atomic(9), i.e. can multiple threads access/mangle the value of td_dom_rr_idx in parallel?
- In vm_domain_policy_validate, couldn’t you remove all of the intermediary `return (-1)`’s as long as you put `break;`s in the switch/case statements?
- Should vm_domain_policy_cleanup/vm_domain_policy_init be implemented? If so, what should they have in there?
- Would it make sense for `struct vm_domain_iterator` to be a queue(9)-like type (just based on the name alone)? If not, what data structure do you anticipate it having, e.g. tree, queue, directed graph, etc?
- In vm_domain_iterator_run, vi->n is always decremented after the vi->n <= 0 run is done — why not move that outside the switch-case statement?
- Why did you hand roll `vm_domain_iterator_isdone` in `vm_domain_iterator_run` up at the top?
- The parameter names for functions/syscalls can be omitted in the declarations.
- The change to vm_page.c seems like it could be committed separate from the NUMA changes.
- `However without it'll kernel panic below - the code didn’t` -> `However without the following check, the kernel will panic below; the code didn’t`
- vm_domain_iterator_run seems like it could use one of the queue(9) data structures.
- Why is OPT_TID 1001?
- `int error;` should come after `cpuset_t set;` per alignment and style(9).
- `atoi` parsing is better handled via strtoll, etc.

Thanks!
-NGie
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20150707/648cfdcf/attachment.bin>