Date: Sat, 14 Oct 2017 17:16:08 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Svatopluk Kraus <skra@freebsd.org> 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: <CAGudoHHbJZHtDiFEN3ckJ62aY-gKd9y4bG3O-PHP6xBKHwzrGA@mail.gmail.com> In-Reply-To: <CAFHCsPWw%2Bur_dEkNO91xpUYQO7rG_w5NjDd8LYgHPuQEL14Ltg@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> <CAFHCsPWw%2Bur_dEkNO91xpUYQO7rG_w5NjDd8LYgHPuQEL14Ltg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Oct 14, 2017 at 11:23 AM, Svatopluk Kraus <skra@freebsd.org> wrote: > On Sat, Oct 14, 2017 at 2:48 AM, Mateusz Guzik <mjguzik@gmail.com> wrote: > > Justification for the change was provided in the commit message: it makes > > .text smaller on amd64 and probably all architectures. > > > > 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. > ----------------- > I guess that's a fair point, I figured some people may be wondering why switching to 0 makes any difference. On the other hand anyone worried about var usage can easily grep. Next time I'll be more verbose. > > 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. > > The code in syscons looks extremely fishy, I don't know what to do with it yet. Testing for ownership should be avoidable with trylock. What you are proposing is an equivalent to the previous trick, but requires reading the lock owner. All places testing for MTX_DESTROYED would have to be updated which is an avoidable churn right now. Not employing the hack makes the flags self-explanatory. If a need for a new flag arises there are still 2 free bits and if that's not enough we can alter the current code. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHHbJZHtDiFEN3ckJ62aY-gKd9y4bG3O-PHP6xBKHwzrGA>