Date: Sat, 14 Oct 2017 11:23:34 +0200 From: Svatopluk Kraus <skra@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Ian Lepore <ian@freebsd.org>, Mateusz Guzik <mjg@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r324609 - head/sys/sys Message-ID: <CAFHCsPWw%2Bur_dEkNO91xpUYQO7rG_w5NjDd8LYgHPuQEL14Ltg@mail.gmail.com> In-Reply-To: <CAGudoHEZ5hdoaPF-PnPW2D397nkOMa4NX7jDB47EzfN4m9AvPA@mail.gmail.com> References: <201710132031.v9DKVueS089009@repo.freebsd.org> <CAFHCsPUzYewo1AaaKFE9LKVdUEw3Tr7c=Qpb=ah9XpwZ-OEEtg@mail.gmail.com> <1507941050.8386.88.camel@freebsd.org> <CAGudoHEZ5hdoaPF-PnPW2D397nkOMa4NX7jDB47EzfN4m9AvPA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Oct 14, 2017 at 2:48 AM, Mateusz Guzik <mjguzik@gmail.com> wrote: > On Sat, Oct 14, 2017 at 2:30 AM, Ian Lepore <ian@freebsd.org> wrote: >> >> On Fri, 2017-10-13 at 23:38 +0200, Svatopluk Kraus wrote: >> > MTX_UNOWNED is a flag. You did not change its value from 4 to 0, you >> > removed it actually. I have very bad feeling about it. But maybe, it's >> > really possible and in that case, a very good explanation should be >> > provided. >> > >> > Svata >> > >> >> The part that scares me is that DESTROYED may have been defined as >> CONTESTED|UNOWNED for subtle and clever reasons lost in the mists of >> time. Any of the places that are testing the MTX_CONTESTED bit may >> have been relying somehow on the fact that a destroyed mutex has that >> bit set. >> >> Then again, maybe Mateusz has carefully analyzed all this stuff, and we >> should just relax. :) >> > > I highly doubt there was anything clever going on. Rather, someone wanted > to somehow denote a destroyed mutex, but did not want to introduce an > additional flag - they are not free since the field shares the value with > the > address of the owner. That is, adding flags requires increasing alignment > of struct thread and that adds to memory usage. Thus they combined the > flags in a way which can never happen under normal circumstances. > > mtx_destroy explicitly sets the value to MTX_DESTROYED, so there is no > change here. > > MTX_UNOWNED is *not* used as a flag. If you grep the tree you will see > it is only used in direct comparisons. > > That said, I reviewed all users again and a minor bug was introduced in > owner_mtx (only used by dtrace). For some reason it grabs the owner, > but the instead of checking if it grabbed anything it checks the flag > (fixed in r324613). So that's my bad. > > Justification for the change was provided in the commit message: it makes > .text smaller on amd64 and probably all architectures. > > -- > Mateusz Guzik <mjguzik gmail.com> I did not lack an explanation why you did it, but why it's possible and safe. Something like this: ----------------- (1) MTX_UNOWNED even if defined like a flag was never used like a flag. It was used like a value set for unowned mutex and test accordingly. There is no logical operation (AND, OR, ...) done with MTX_UNOWNED in code. There are only assignments and checks for equality there. Thus, we can change its value and do not pretend that it's a flag anymore. As mutex owner thread pointer and mutex flags are kept in one variable together, setting MTX_UNOWNED value to 0 is more appropriate and brings some benefits. However, MTX_DESTROYED flag must be distinguish from MTX_CONTESTED one now, so there is no savings considering mutex flags. (2) MTX_UNONWED value is used only internally within mutex implemention, thus this change should be quite safe. ----------------- However, I suggest to do something with MTX_UNOWNED leak in sys/dev/syscons/syscons.c. i.e. to make mtx_unowned() public and use it instead. Further, MTX_DESTROYED is not used like a flag too. So, was its redefinition really needed? In other words, isn't MTX_CONTESTED flag without owner thread pointer set enough? Also, I would prefer to have some explanation to be in mutex.h about all of this. At least to not mess flags with values in definitions without explanation. Svata
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFHCsPWw%2Bur_dEkNO91xpUYQO7rG_w5NjDd8LYgHPuQEL14Ltg>