From owner-freebsd-arch@freebsd.org Wed Jul 8 19:44:37 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 1A419996C16 for ; Wed, 8 Jul 2015 19:44:37 +0000 (UTC) (envelope-from bright@mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 079CE165A; Wed, 8 Jul 2015 19:44:36 +0000 (UTC) (envelope-from bright@mu.org) Received: from [10.2.8.86] (unknown [50.204.88.51]) by elvis.mu.org (Postfix) with ESMTPSA id 3C8EE341F877; Wed, 8 Jul 2015 12:44:35 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (1.0) Subject: Re: CFT/CFR: NUMA policy branch From: Alfred Perlstein X-Mailer: iPhone Mail (12H143) In-Reply-To: <168418DA-CCD3-4BA8-A0C9-5F0CF967F2E0@bsdimp.com> Date: Wed, 8 Jul 2015 12:44:34 -0700 Cc: Alfred Perlstein , Adrian Chadd , Garrett Cooper , "freebsd-arch@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: References: <559D778B.5050408@freebsd.org> <168418DA-CCD3-4BA8-A0C9-5F0CF967F2E0@bsdimp.com> To: Warner Losh 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:44:37 -0000 What name calling? Sent from my iPhone > On Jul 8, 2015, at 12:33 PM, Warner Losh wrote: >=20 >=20 >> On Jul 8, 2015, at 1:18 PM, Alfred Perlstein wrote: >>=20 >>=20 >>=20 >>> On 7/7/15 11:38 PM, Adrian Chadd wrote: >>> There's a phabricator review. It's not up to date, because: >>>=20 >>> * it broke for a while, and >>> * kib requested he be sent patches, not a phabricator review. >>=20 >>=20 >> So Kib is complaining that his feedback is getting lost, but refuses to u= se a review tracker? >>=20 >> MFW: >> http://images5.fanpop.com/image/photos/25000000/No-country-for-old-men-to= mmy-lee-jones-25069727-450-276.jpg >=20 > Do we really need to resort to name calling for a reviewer who is trying > to help make things better? kib has provided me good feedback on patches > I=E2=80=99ve done in the past, though it sometimes takes me a while to und= erstand > his concerns. It is well worth the while to do that, and to engage him > constructively rather than belittling his efforts. And experience has show= n > that phabricator is great for small patches, but terrible for large patche= s > that get revised over and over before going in. This is the 4th or 5th > review than I can recall where phabricator=E2=80=99s flaws went from minor= > annoyances to major hassles. For really big reviews, I=E2=80=99m starting t= o think > after 2 or 3 rounds we should close the review and start a new one > to help work around the issues. >=20 > In other words, the right reaction to =E2=80=9CI=E2=80=99m stopping the re= view here since it isn=E2=80=99t > half done=E2=80=9D isn=E2=80=99t the defensive and belittling one one I=E2= =80=99ve seen, but rather to > start a conversation about what he thinks is missing. Maybe he=E2=80=99s m= issed > something, or maybe you have. While it is cool people are using it, kib=E2= =80=99s > concerns generally are looking past the initial glow of partial success fo= r > how to climb the next mountain range and generally are worth the effort > to get. >=20 > Warner >=20 >> -Alfred >>=20 >>>=20 >>> -a >>>=20 >>>=20 >>>> On 7 July 2015 at 23:13, Garrett Cooper wrote: >>>>> On Jul 5, 2015, at 19:06, Adrian Chadd wrote: >>>>>=20 >>>>> Hi, >>>>>=20 >>>>> I've done another update. kib@ has been beating me with the clue stick= a bit. >>>>>=20 >>>>> https://github.com/freebsd/freebsd/compare/master...erikarn:local/adri= an_numa_policy >>>>>=20 >>>>> * (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 >>>>>=20 >>>>> I've tested the policies (first-touch, fixed-domain, round-robin) and >>>>> they all still work as advertised, both on threads and processes. >>>>>=20 >>>>> I'd appreciate more reviews and some further testing. >>>> Please create a dummy pull request or post the code up on Phabricator. >>>>=20 >>>> - 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_poli= cy 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_nu= ma_getaffinity, but not sys_numa_setaffinity? >>>> - sys_numa_setaffinity and sys_numa_getaffinity look similar. Could som= ething be implemented like sysctl(3) for handling getting/setting of affinit= ies 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 acr= oss 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 mul= tiple 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 i= ntermediary `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?= 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 <=3D 0 run is done =E2=80=94 why not move that outside the switch-case sta= tement? >>>> - Why did you hand roll `vm_domain_iterator_isdone` in `vm_domain_itera= tor_run` up at the top? >>>> - The parameter names for functions/syscalls can be omitted in the decl= arations. >>>> - The change to vm_page.c seems like it could be committed separate fro= m 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) da= ta structures. >>>> - Why is OPT_TID 1001? >>>> - `int error;` should come after `cpuset_t set;` per alignment and styl= e(9). >>>> - `atoi` parsing is better handled via strtoll, etc. >>>>=20 >>>> 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" >>=20 >> _______________________________________________ >> 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" >=20