From owner-freebsd-arch@freebsd.org Wed Jul 8 19:34:02 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 3375C9969A7 for ; Wed, 8 Jul 2015 19:34:02 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from mail-pa0-f47.google.com (mail-pa0-f47.google.com [209.85.220.47]) (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 055CA1E0A for ; Wed, 8 Jul 2015 19:34:01 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: by pabvl15 with SMTP id vl15so136540039pab.1 for ; Wed, 08 Jul 2015 12:33:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:subject:mime-version:content-type:from :in-reply-to:date:cc:message-id:references:to; bh=9TfwR0q4RaU9A4zU9k3N+b0dV/+KFKUnfJtNfInRanA=; b=E1NCVoWwTPivW09TVDMvvDyrFH9WnxGVOIzMIkgGLdzeVvr4h++6p5+5Y9MsO27y1i 1BjjPmxe4MIYHuGdHhqQzwFsILUTfPfa9ej98V8YWyozjgAXDi/GQSRR8VaLm5R96euR 8hGnqV2bLa4EWgx1TAE8pnsYQi0OcJnqeby72Ww8ujli8LV1k++tWOl0zgfsRpE3Pla/ TIHxDqSncGL++nTah15AmWCgg+50a7OiG3DlOPXfOetUztNk2EziVpWxoV3OzTRnHr6B KxA1fYh1u6DNae/12orhy2raMSeXetrATITyqsVNTLHgthTu//W/DTNfNplWjJbEzKiU eMDA== X-Gm-Message-State: ALoCoQmLz/yKS3UMOOkwoOUlvN95x6+bUfrcCMyovYDBrF3xpRYNWBLYIrKc4mMeI2XiWdk+4VXG X-Received: by 10.70.102.164 with SMTP id fp4mr23911254pdb.48.1436384035444; Wed, 08 Jul 2015 12:33:55 -0700 (PDT) Received: from [10.64.26.63] ([69.53.236.236]) by smtp.gmail.com with ESMTPSA id om10sm3347774pbb.58.2015.07.08.12.33.53 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 08 Jul 2015 12:33:54 -0700 (PDT) Sender: Warner Losh Subject: Re: CFT/CFR: NUMA policy branch Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\)) Content-Type: multipart/signed; boundary="Apple-Mail=_A32F3F2F-3D8E-42E7-903C-EFE2F1DBAF21"; protocol="application/pgp-signature"; micalg=pgp-sha512 X-Pgp-Agent: GPGMail 2.5 From: Warner Losh In-Reply-To: <559D778B.5050408@freebsd.org> Date: Wed, 8 Jul 2015 13:33:40 -0600 Cc: Adrian Chadd , Garrett Cooper , "freebsd-arch@freebsd.org" Message-Id: <168418DA-CCD3-4BA8-A0C9-5F0CF967F2E0@bsdimp.com> References: <559D778B.5050408@freebsd.org> To: Alfred Perlstein X-Mailer: Apple Mail (2.2098) 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:34:02 -0000 --Apple-Mail=_A32F3F2F-3D8E-42E7-903C-EFE2F1DBAF21 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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 >=20 > So Kib is complaining that his feedback is getting lost, but refuses = to use a review tracker? >=20 > MFW: > = http://images5.fanpop.com/image/photos/25000000/No-country-for-old-men-tom= my-lee-jones-25069727-450-276.jpg 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 = understand his concerns. It is well worth the while to do that, and to engage him constructively rather than belittling his efforts. And experience has = shown that phabricator is great for small patches, but terrible for large = patches 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 to think after 2 or 3 rounds we should close the review and start a new one to help work around the issues. In other words, the right reaction to =E2=80=9CI=E2=80=99m stopping the = review 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 = missed 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 = for how to climb the next mountain range and generally are worth the effort to get. Warner > -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/adrian_n= uma_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_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 = something be implemented like sysctl(3) for handling getting/setting of = affinities 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 = 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=E2=80=99t you remove all of = the intermediary `return (-1)`=E2=80=99s 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 <=3D 0 run is done =E2=80=94 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=E2=80=99t`= -> `However without the following check, the kernel will panic below; = the code 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. >>>=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" --Apple-Mail=_A32F3F2F-3D8E-42E7-903C-EFE2F1DBAF21 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJVnXsVAAoJEGwc0Sh9sBEAPlUQANgVqYekRlZgk2KoFLVwoZzW 6rMEKLC0SSW9LmcH2fpx8w0OZ+7DenCzEe7Szcn7sQzEsfjACgOEcziRnuMjlhTw Kay0jLiGviFm2Y6c6CV4JmE9D+vvNMpASUzD6+Mx1UHh7cbB9kWXYvuD4YZfYuEv L6rcipAWar7jsf7rl3Ez/fdMQVH2E1QtNm0Yrg1ZOV09+ENkxof0b/tJytt6iCAs ISKeSuDlXbZ0uj6NawtZ3qTafCjMb1ayccsXFQHizmBjxWqMY1mZwk3tOCLjgsZZ O7Ix/GZgF5ic4CDNXNNf/Ww3FEcYaplK1r3yia54wEwZYWPIotuCVB+eRei3p/sX mCdtAlwJa6XD+1B3uS7pHMnK7Zeg9adySDKZdnLy72RVr2jSPDYrFtEx4mkAoq3h 4jYKfnIVL4bbreTnp1Qik2HuwZkl39AwVGbJpm/49CM3Y4OVO5r1sehWBZjX028i /Yudo6p9tDn9YBpVR47OWnSCvMmyG/4g4HLbhGJjfsOfcO/ZVsgKWyDVgitzBSxx iO4qQwT4UpiUWwgqBxdpVU5YcewPRjM0Bc+eh1Vv8Pqj8JUZQnmZo/0OCxCt6kXO V90/nXYeYoBAF5lFYgG67iO/qiUinnElllqIqbm8sYrAtQl7dBEvKS2gNqUmjtjw DYmxSb0W2EsHbZUEAC3L =qAlQ -----END PGP SIGNATURE----- --Apple-Mail=_A32F3F2F-3D8E-42E7-903C-EFE2F1DBAF21--