CFT/CFR: NUMA policy branch

[Warner Losh]

> On Jul 8, 2015, at 1:18 PM, Alfred Perlstein <alfred at 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 at gmail.com> wrote:
>>> On Jul 5, 2015, at 19:06, Adrian Chadd <adrian at 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 at freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
>> To unsubscribe, send any mail to "freebsd-arch-unsubscribe at freebsd.org"
> 
> _______________________________________________
> freebsd-arch at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscribe at freebsd.org"

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20150708/f0915448/attachment.bin>