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>
index | next in thread | previous in thread | raw e-mail
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. Svatahome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFHCsPWw%2Bur_dEkNO91xpUYQO7rG_w5NjDd8LYgHPuQEL14Ltg>