svn commit: r324609 - head/sys/sys

[Mateusz Guzik]

On Sat, Oct 14, 2017 at 11:23 AM, Svatopluk Kraus <skra at freebsd.org> wrote:

> On Sat, Oct 14, 2017 at 2:48 AM, Mateusz Guzik <mjguzik at 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>