From owner-freebsd-arch@freebsd.org Wed Jul 8 06:38:11 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 08B149960F5 for ; Wed, 8 Jul 2015 06:38:11 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-ig0-x234.google.com (mail-ig0-x234.google.com [IPv6:2607:f8b0:4001:c05::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id C5EF81681 for ; Wed, 8 Jul 2015 06:38:10 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: by iggp10 with SMTP id p10so14891183igg.0 for ; Tue, 07 Jul 2015 23:38:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=XVsgEyaZ9PDJwwk2GUZvd8dPnm2HDChAO3QcqcaSyCQ=; b=Bph9UZAaCcatZDjG7d1NTJdPCevVyqysEjCbf2CSrJYPyMy9sehOhB0oWOg/NKmStg BrsHe72iFmJ4OJShhViWQg5FhSm89HUCk9ExmiY2hvmIJxsycgOjEvH/JXBbmxU0BPL9 XSJGTdvQWUPwVQOJZEM2C55BSgpIAq1Mecww1iAVSzcsqsHEvhEejwUk/D8lLOVNIM6g Jz7zcFqJopDQGX5tqrmX12hUn7ac3pK2wM7fk9Peur3BAirEpqJFFiDLzhyD59LDBLeF njGGCCEHGZBXadr2Bb8sorPlP99MluMeF4PkdbH7j2Pf1I1a6gWZOeeJlaiU7yqr7WFe 7Ndw== MIME-Version: 1.0 X-Received: by 10.107.155.74 with SMTP id d71mr13796318ioe.29.1436337490210; Tue, 07 Jul 2015 23:38:10 -0700 (PDT) Sender: adrian.chadd@gmail.com Received: by 10.36.38.133 with HTTP; Tue, 7 Jul 2015 23:38:10 -0700 (PDT) In-Reply-To: References: Date: Tue, 7 Jul 2015 23:38:10 -0700 X-Google-Sender-Auth: mUbZFW01ie5RLTqev7tsAyu5KTE Message-ID: Subject: Re: CFT/CFR: NUMA policy branch From: Adrian Chadd To: Garrett Cooper Cc: "freebsd-arch@freebsd.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 06:38:11 -0000 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. -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=3D16` 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 somet= hing be implemented like sysctl(3) for handling getting/setting of affiniti= es all in one shot? > - `if (p)` should be `if (p !=3D NULL)`, etc per style(9). > - Is there a way that the affinity could be inherited/not inherited acros= s 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 multi= ple threads access/mangle the value of td_dom_rr_idx in parallel? > - In vm_domain_policy_validate, couldn=E2=80=99t you remove all of the in= termediary `return (-1)`=E2=80=99s as long as you put `break;`s in the swit= ch/case statements? > - Should vm_domain_policy_cleanup/vm_domain_policy_init be implemented? I= f so, what should they have in there? > - Would it make sense for `struct vm_domain_iterator` to be a queue(9)-li= ke 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 = <=3D 0 run is done =E2=80=94 why not move that outside the switch-case stat= ement? > - Why did you hand roll `vm_domain_iterator_isdone` in `vm_domain_iterato= r_run` up at the top? > - The parameter names for functions/syscalls can be omitted in the declar= ations. > - 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=E2=80=99t` ->= `However without the following check, the kernel will panic below; the cod= e didn=E2=80=99t` > - 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