Skip site navigation (1)Skip section navigation (2)
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>

next in thread | previous in thread | raw e-mail | index | archive | help

--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 <alfred@freebsd.org> =
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 <yaneurabeya@gmail.com> =
wrote:
>>> On Jul 5, 2015, at 19:06, Adrian Chadd <adrian@freebsd.org> 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--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?168418DA-CCD3-4BA8-A0C9-5F0CF967F2E0>