From owner-freebsd-arch@freebsd.org Wed Jul 8 19:16:09 2015 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C5558996520 for ; Wed, 8 Jul 2015 19:16:09 +0000 (UTC) (envelope-from alfred@freebsd.org) Received: from elvis.mu.org (elvis.mu.org [IPv6:2001:470:1f05:b76::196]) by mx1.freebsd.org (Postfix) with ESMTP id B28E9167D; Wed, 8 Jul 2015 19:16:09 +0000 (UTC) (envelope-from alfred@freebsd.org) Received: from u10-2-32-011.office.norse-data.com (unknown [50.204.88.51]) by elvis.mu.org (Postfix) with ESMTPSA id 4A634341F877; Wed, 8 Jul 2015 12:16:09 -0700 (PDT) Message-ID: <559D778B.5050408@freebsd.org> Date: Wed, 08 Jul 2015 12:18:35 -0700 From: Alfred Perlstein Organization: FreeBSD User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Adrian Chadd , Garrett Cooper CC: "freebsd-arch@freebsd.org" Subject: Re: CFT/CFR: NUMA policy branch References: In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jul 2015 19:16:10 -0000 On 7/7/15 11:38 PM, Adrian Chadd wrote: > There's a phabricator review. It's not up to date, because: > > * it broke for a while, and > * kib requested he be sent patches, not a phabricator review. > So Kib is complaining that his feedback is getting lost, but refuses to use a review tracker? MFW: http://images5.fanpop.com/image/photos/25000000/No-country-for-old-men-tommy-lee-jones-25069727-450-276.jpg -Alfred > > -a > > > On 7 July 2015 at 23:13, Garrett Cooper wrote: >> On Jul 5, 2015, at 19:06, Adrian Chadd 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 > _______________________________________________ > freebsd-arch@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"