CFT/CFR: NUMA policy branch

Wed Jul 8 19:44:37 UTC 2015

What name calling?

Sent from my iPhone

> On Jul 8, 2015, at 12:33 PM, Warner Losh <imp at bsdimp.com> wrote:
> 
> 
>> 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"
> 