CFT/CFR: NUMA policy branch

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

CFT/CFR: NUMA policy branch

Adrian Chadd-2
Hi,

I'm at the point now where I think this part is done and I'd like to
get it more formally reviewed and ready for commit into -HEAD.

Here's the branch:

https://github.com/freebsd/freebsd/compare/master...erikarn:local/adrian_numa_policy

* added syscalls;
* added vm domain policy and iterators;
* per-process, per-thread and global system policy;
* added numactl - a tool to manipulate, fetch and start processes w/ policies;;
* added manpages for everything above.

It's working fine for a handful of people who have been testing it.
I'd like to get some more wider testing and reporting.

I'd like to wrap this up in a formal phabricator review in the next
few days and kick start it along.

Thanks!


-adrian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
Hi,

It's in a review, and I've addressed most of the feedback:

https://reviews.freebsd.org/D2559

I'd like to get more feedback before I commit this. I'm hoping to
commit it next week so I can make a start on pushing numa awareness
into KVA allocation and contigmalloc/busdma.

Thanks!



-adrian


On 12 May 2015 at 18:32, Adrian Chadd <[hidden email]> wrote:

> Hi,
>
> I'm at the point now where I think this part is done and I'd like to
> get it more formally reviewed and ready for commit into -HEAD.
>
> Here's the branch:
>
> https://github.com/freebsd/freebsd/compare/master...erikarn:local/adrian_numa_policy
>
> * added syscalls;
> * added vm domain policy and iterators;
> * per-process, per-thread and global system policy;
> * added numactl - a tool to manipulate, fetch and start processes w/ policies;;
> * added manpages for everything above.
>
> It's working fine for a handful of people who have been testing it.
> I'd like to get some more wider testing and reporting.
>
> I'd like to wrap this up in a formal phabricator review in the next
> few days and kick start it along.
>
> Thanks!
>
>
> -adrian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Warner Losh
Hi Adrian,

How does this work release to the branch that Jeff Roberson had for numaizing
the VM system? Can any of that work be carried forward?

Warner


> On May 31, 2015, at 9:47 PM, Adrian Chadd <[hidden email]> wrote:
>
> Hi,
>
> It's in a review, and I've addressed most of the feedback:
>
> https://reviews.freebsd.org/D2559
>
> I'd like to get more feedback before I commit this. I'm hoping to
> commit it next week so I can make a start on pushing numa awareness
> into KVA allocation and contigmalloc/busdma.
>
> Thanks!
>
>
>
> -adrian
>
>
> On 12 May 2015 at 18:32, Adrian Chadd <[hidden email]> wrote:
>> Hi,
>>
>> I'm at the point now where I think this part is done and I'd like to
>> get it more formally reviewed and ready for commit into -HEAD.
>>
>> Here's the branch:
>>
>> https://github.com/freebsd/freebsd/compare/master...erikarn:local/adrian_numa_policy
>>
>> * added syscalls;
>> * added vm domain policy and iterators;
>> * per-process, per-thread and global system policy;
>> * added numactl - a tool to manipulate, fetch and start processes w/ policies;;
>> * added manpages for everything above.
>>
>> It's working fine for a handful of people who have been testing it.
>> I'd like to get some more wider testing and reporting.
>>
>> I'd like to wrap this up in a formal phabricator review in the next
>> few days and kick start it along.
>>
>> Thanks!
>>
>>
>> -adrian
> _______________________________________________
> [hidden email] mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]"


signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
Where's it hiding? All I have seen is the projects/numa branch. This is a
subset of that.

Adrian
 On Jun 1, 2015 8:38 AM, "Warner Losh" <[hidden email]> wrote:

> Hi Adrian,
>
> How does this work release to the branch that Jeff Roberson had for
> numaizing
> the VM system? Can any of that work be carried forward?
>
> Warner
>
>
> > On May 31, 2015, at 9:47 PM, Adrian Chadd <[hidden email]> wrote:
> >
> > Hi,
> >
> > It's in a review, and I've addressed most of the feedback:
> >
> > https://reviews.freebsd.org/D2559
> >
> > I'd like to get more feedback before I commit this. I'm hoping to
> > commit it next week so I can make a start on pushing numa awareness
> > into KVA allocation and contigmalloc/busdma.
> >
> > Thanks!
> >
> >
> >
> > -adrian
> >
> >
> > On 12 May 2015 at 18:32, Adrian Chadd <[hidden email]> wrote:
> >> Hi,
> >>
> >> I'm at the point now where I think this part is done and I'd like to
> >> get it more formally reviewed and ready for commit into -HEAD.
> >>
> >> Here's the branch:
> >>
> >>
> https://github.com/freebsd/freebsd/compare/master...erikarn:local/adrian_numa_policy
> >>
> >> * added syscalls;
> >> * added vm domain policy and iterators;
> >> * per-process, per-thread and global system policy;
> >> * added numactl - a tool to manipulate, fetch and start processes w/
> policies;;
> >> * added manpages for everything above.
> >>
> >> It's working fine for a handful of people who have been testing it.
> >> I'd like to get some more wider testing and reporting.
> >>
> >> I'd like to wrap this up in a formal phabricator review in the next
> >> few days and kick start it along.
> >>
> >> Thanks!
> >>
> >>
> >> -adrian
> > _______________________________________________
> > [hidden email] mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> > To unsubscribe, send any mail to "[hidden email]"
>
>
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
In reply to this post by Adrian Chadd-2
Hi,

I've updated the NUMA branch again:

https://github.com/freebsd/freebsd/compare/master...erikarn:local/adrian_numa_policy

The main fixes:

* (stas) Added short versions of options to numactl;
* (stas, wblock, rpaulo) Documentation and code fixes during review;
* (kib) updated the userland facing API to not include seq_t;
* fixed up a couple of silly bugs that gcc-4.2 picked up;
* fixed up compile issues on gcc-4.2.

I've tested this on mips32, mips64, x86 non-NUMA (GENERIC) and NUMA hardware.

kib@ has requested that this use the procctl() API rather than adding
new syscalls. procctl() currently doesn't support P_LWPID (ie thread)
based identifiers for any of its manipulation, so I'd have to go and
add that.

I think this is close to what I'd like to commit. I'd appreciate another review.

Thanks,


-adrian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
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.

Thanks!



-adrian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Konstantin Belousov
On Sun, Jul 05, 2015 at 07:06:27PM -0700, Adrian Chadd 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.

I did not found a fix for the wrong locking of seq_t.
Am I reading sys_numa_getaffinity() right that it does copyout() while
owning the process lock ?
The things are still syscalls instead of procctl() commands.

I did not read further, the patch is half-done at best.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
On 7 July 2015 at 02:37, Konstantin Belousov <[hidden email]> wrote:

> On Sun, Jul 05, 2015 at 07:06:27PM -0700, Adrian Chadd 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.
>
> I did not found a fix for the wrong locking of seq_t.

Which locking is wrong?

* the global policy is locked via the global mutex;
* the process/thread policy is locked via PROC_LOCK.

What did I miss?

> Am I reading sys_numa_getaffinity() right that it does copyout() while
> owning the process lock ?

I've fixed and push that, thanks.

> The things are still syscalls instead of procctl() commands.

I haven't done that yet. procctl() doesn't have a concept of per-TID
operations at all, and it currently has a framework for iterating over
things that this would have to slot into somehow. I'm open to
suggestions on how you would integrate per-TID operations into
procctl().

> I did not read further, the patch is half-done at best.

That's lovely. Meanwhile, people are actively using this thing.




-adrian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Rui Paulo-6
On Tuesday 07 July 2015 15:53:18 Adrian Chadd wrote:

> > I did not read further, the patch is half-done at best.
>
> That's lovely. Meanwhile, people are actively using this thing.

It may not be perfect, but it's way more than half done.  You might object to
introducing the syscalls, but procctl is still annoyingly limited.

--
Rui Paulo
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Alfred Perlstein-2
In reply to this post by Konstantin Belousov


On 7/7/15 2:37 AM, Konstantin Belousov wrote:
> I did not read further, the patch is half-done at best.

This is unproductive.

People are using Adrian's work and we plan on using it as well.

FreeBSD is behind in the NUMA game by at least a decade

Please describe in detail what the issues are, do not roadblock based on
vagueshedding.

JFYI:
 From http://kernelnewbies.org/LinuxVersions ->

2.5.59 released Janury 17, 2003:
NUMA aware scheduler extensions

Let's move on and incrementally fix this as opposed to roadblocking.  
It's 13 years late.

-Alfred
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Alfred Perlstein-2
In reply to this post by Rui Paulo-6


On 7/7/15 7:43 PM, Rui Paulo wrote:
> On Tuesday 07 July 2015 15:53:18 Adrian Chadd wrote:
>
>>> I did not read further, the patch is half-done at best.
>> That's lovely. Meanwhile, people are actively using this thing.
> It may not be perfect, but it's way more than half done.  You might object to
> introducing the syscalls, but procctl is still annoyingly limited.
>
(not yelling at you Rui)... but really... Is that the problem?!!? Just
write a userland library to abstract the kernel interface!

-Alfred
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Enji Cooper
In reply to this post by Adrian Chadd-2
On Jul 5, 2015, at 19:06, Adrian Chadd <[hidden email]> 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

signature.asc (507 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
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.



-a


On 7 July 2015 at 23:13, Garrett Cooper <[hidden email]> wrote:

> On Jul 5, 2015, at 19:06, Adrian Chadd <[hidden email]> 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
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Rui Paulo-6
In reply to this post by Alfred Perlstein-2
On Tuesday 07 July 2015 22:33:19 Alfred Perlstein wrote:

> On 7/7/15 7:43 PM, Rui Paulo wrote:
> > On Tuesday 07 July 2015 15:53:18 Adrian Chadd wrote:
> >>> I did not read further, the patch is half-done at best.
> >>
> >> That's lovely. Meanwhile, people are actively using this thing.
> >
> > It may not be perfect, but it's way more than half done.  You might object
> > to introducing the syscalls, but procctl is still annoyingly limited.
> (not yelling at you Rui)... but really... Is that the problem?!!? Just
> write a userland library to abstract the kernel interface!

How can a library help?  If you can't tell the kernel to apply a policy per-
TID (procctl works by PID), it's useless for multi-threaded applications.

--
Rui Paulo
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Alfred Perlstein-3


> On Jul 7, 2015, at 11:44 PM, Rui Paulo <[hidden email]> wrote:
>
>> On Tuesday 07 July 2015 22:33:19 Alfred Perlstein wrote:
>>> On 7/7/15 7:43 PM, Rui Paulo wrote:
>>> On Tuesday 07 July 2015 15:53:18 Adrian Chadd wrote:
>>>>> I did not read further, the patch is half-done at best.
>>>>
>>>> That's lovely. Meanwhile, people are actively using this thing.
>>>
>>> It may not be perfect, but it's way more than half done.  You might object
>>> to introducing the syscalls, but procctl is still annoyingly limited.
>> (not yelling at you Rui)... but really... Is that the problem?!!? Just
>> write a userland library to abstract the kernel interface!
>
> How can a library help?  If you can't tell the kernel to apply a policy per-
> TID (procctl works by PID), it's useless for multi-threaded applications.
>

The library would abstract away the kernel boundary concerns. So let's say we did what kib wanted and extended procctl to support tids, well at that point the syscalls made could be garbage collected and the library updated to call the correct kernel entry point.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Alfred Perlstein-2
In reply to this post by Adrian Chadd-2


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

-Alfred

>
> -a
>
>
> On 7 July 2015 at 23:13, Garrett Cooper <[hidden email]> wrote:
>> On Jul 5, 2015, at 19:06, Adrian Chadd <[hidden email]> 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
> _______________________________________________
> [hidden email] mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]"

_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Warner Losh

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


signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Alfred Perlstein-3
What name calling?

Sent from my iPhone

> On Jul 8, 2015, at 12:33 PM, Warner Losh <[hidden email]> wrote:
>
>
>> On Jul 8, 2015, at 1:18 PM, Alfred Perlstein <[hidden email]> 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 <[hidden email]> wrote:
>>>>> On Jul 5, 2015, at 19:06, Adrian Chadd <[hidden email]> 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
>>> _______________________________________________
>>> [hidden email] mailing list
>>> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
>>> To unsubscribe, send any mail to "[hidden email]"
>>
>> _______________________________________________
>> [hidden email] mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-arch
>> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Andriy Gapon
In reply to this post by Alfred Perlstein-2
On 08/07/2015 22:18, Alfred Perlstein 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?

How about phabricator losing diffs? Would that be a valid complaint?

P.S.

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: CFT/CFR: NUMA policy branch

Adrian Chadd-2
On 8 July 2015 at 13:43, Andriy Gapon <[hidden email]> wrote:

> On 08/07/2015 22:18, Alfred Perlstein 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?
>
> How about phabricator losing diffs? Would that be a valid complaint?

Hi,

Let's not get side tracked. I've invited a variety of people to review
and comment on this stuff. Some people want it in reviews.freebsd.org,
some want it via diffs. Different people want work done in different
units of work.

My plan is to get this into "good enough" state to throw into -HEAD. I
don't even care if in 12 months it's completely replaced with an
alternate implementation and/or API. What i care about right now is
getting the basic pieces in place so further work and experimentation
can be done. Right now the entry limit to evaluating any NUMA things
on FreeBSD is "you don't, without numa.diff", and that's unacceptable.
I completely expect that it'll change over the course of a few years.
But the fact we still don't have even the most basic userland exposed
API for controlling things is IMHO unacceptable and reflects poorly on
us as a community.

So, I'm looking for less nit-picking and more "this is wrong, you
should do this." A lot of kibs responses have been errors on my part
that I hadn't picked up on and weren't exposed during testing. I'm
looking for more of those. I haven't yet gone over Garrett's comments
in too much depth; I'll look at that tonight if I don't fall asleep
first.

The important thing here is to try and finally move the default
available functionality along a little bit so people can get
interested and start using this and contribute their own work.

Thanks,



-adrian
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
12