Date: Wed, 8 Jul 2015 13:33:40 -0600 From: Warner Losh <imp@bsdimp.com> To: Alfred Perlstein <alfred@freebsd.org> Cc: Adrian Chadd <adrian@freebsd.org>, Garrett Cooper <yaneurabeya@gmail.com>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: CFT/CFR: NUMA policy branch Message-ID: <168418DA-CCD3-4BA8-A0C9-5F0CF967F2E0@bsdimp.com> In-Reply-To: <559D778B.5050408@freebsd.org> References: <CAJ-Vmo=SnqXTF5m65haKqrVf699zinyXs%2BQdvR6V88CW7vooCw@mail.gmail.com> <CAJ-VmonyTfSxj%2BD=FN3TUCO33w4vGqh1REQqx-8rd-JcArfqSA@mail.gmail.com> <CAJ-Vmo=ON9bEngDoMFK-kJR=qVvcX%2BEeQXx%2BUoEsh1npMHjESQ@mail.gmail.com> <D60A4F17-D573-4C0D-AD8B-2C3710A67DA6@gmail.com> <CAJ-Vmokym0_M=owRuR1uAcfAnzme-0xULs%2BYShQ7GXa7V9F6Vg@mail.gmail.com> <559D778B.5050408@freebsd.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] > On Jul 8, 2015, at 1:18 PM, Alfred Perlstein <alfred@freebsd.org> wrote: > > > > 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 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’ve 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’s flaws went from minor annoyances to major hassles. For really big reviews, I’m 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 “I’m stopping the review here since it isn’t half done” isn’t the defensive and belittling one one I’ve seen, but rather to start a conversation about what he thinks is missing. Maybe he’s missed something, or maybe you have. While it is cool people are using it, kib’s 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 > >> >> -a >> >> >> On 7 July 2015 at 23:13, Garrett Cooper <yaneurabeya@gmail.com> wrote: >>> On Jul 5, 2015, at 19:06, Adrian Chadd <adrian@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 >> _______________________________________________ >> 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" > > _______________________________________________ > 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" [-- Attachment #2 --] -----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-----home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?168418DA-CCD3-4BA8-A0C9-5F0CF967F2E0>