On Wed, Mar 17, 2021 at 12:38:22PM -0000, tip-bot2 for Waiman Long wrote:
> The following commit has been merged into the locking/urgent branch of tip:
>
> Commit-ID: 5de2055d31ea88fd9ae9709ac95c372a505a60fa
> Gitweb: https://git.kernel.org/tip/5de2055d31ea88fd9ae9709ac95c372a505a60fa
> Author: Waiman Long <[email protected]>
> AuthorDate: Tue, 16 Mar 2021 11:31:16 -04:00
> Committer: Ingo Molnar <[email protected]>
> CommitterDate: Wed, 17 Mar 2021 09:56:44 +01:00
>
> locking/ww_mutex: Simplify use_ww_ctx & ww_ctx handling
>
> The use_ww_ctx flag is passed to mutex_optimistic_spin(), but the
> function doesn't use it. The frequent use of the (use_ww_ctx && ww_ctx)
> combination is repetitive.
>
> In fact, ww_ctx should not be used at all if !use_ww_ctx. Simplify
> ww_mutex code by dropping use_ww_ctx from mutex_optimistic_spin() an
> clear ww_ctx if !use_ww_ctx. In this way, we can replace (use_ww_ctx &&
> ww_ctx) by just (ww_ctx).
The reason this code was like this is because GCC could constant
propagage use_ww_ctx but could not do the same for ww_ctx (since that's
external).
Please double check generated code to make sure you've not introduced a
bunch of extra branches.
On 3/17/21 8:59 AM, Peter Zijlstra wrote:
> On Wed, Mar 17, 2021 at 12:38:22PM -0000, tip-bot2 for Waiman Long wrote:
>> The following commit has been merged into the locking/urgent branch of tip:
>>
>> Commit-ID: 5de2055d31ea88fd9ae9709ac95c372a505a60fa
>> Gitweb: https://git.kernel.org/tip/5de2055d31ea88fd9ae9709ac95c372a505a60fa
>> Author: Waiman Long <[email protected]>
>> AuthorDate: Tue, 16 Mar 2021 11:31:16 -04:00
>> Committer: Ingo Molnar <[email protected]>
>> CommitterDate: Wed, 17 Mar 2021 09:56:44 +01:00
>>
>> locking/ww_mutex: Simplify use_ww_ctx & ww_ctx handling
>>
>> The use_ww_ctx flag is passed to mutex_optimistic_spin(), but the
>> function doesn't use it. The frequent use of the (use_ww_ctx && ww_ctx)
>> combination is repetitive.
>>
>> In fact, ww_ctx should not be used at all if !use_ww_ctx. Simplify
>> ww_mutex code by dropping use_ww_ctx from mutex_optimistic_spin() an
>> clear ww_ctx if !use_ww_ctx. In this way, we can replace (use_ww_ctx &&
>> ww_ctx) by just (ww_ctx).
> The reason this code was like this is because GCC could constant
> propagage use_ww_ctx but could not do the same for ww_ctx (since that's
> external).
>
> Please double check generated code to make sure you've not introduced a
> bunch of extra branches.
>
I see, but this patch just replaces (use_ww_ctx && ww_ctx) by (ww_ctx).
Even if constant propagation isn't happening for ww_ctx, gcc shouldn't
generate any worse code wrt ww_ctx. It could be that the generated
machine code are more or less the same, but the source code will look
cleaner with just one variable in the conditional clauses.
Using gcc 8.4.1, the generated __mutex_lock function has the same size
(with last instruction at offset +5179) with or without this patch.
Well, you can say that this patch is an no-op wrt generated code.
Cheers,
Longman
On Wed, Mar 17, 2021 at 09:43:20AM -0400, Waiman Long wrote:
> Using gcc 8.4.1, the generated __mutex_lock function has the same size (with
> last instruction at offset +5179) with or without this patch. Well, you can
> say that this patch is an no-op wrt generated code.
OK, then GCC has gotten better. Because back then I tried really hard
but it wouldn't remove the if (ww_ctx) branches unless I had that extra
const bool argument.
On 3/17/21 9:55 AM, Peter Zijlstra wrote:
> On Wed, Mar 17, 2021 at 09:43:20AM -0400, Waiman Long wrote:
>
>> Using gcc 8.4.1, the generated __mutex_lock function has the same size (with
>> last instruction at offset +5179) with or without this patch. Well, you can
>> say that this patch is an no-op wrt generated code.
> OK, then GCC has gotten better. Because back then I tried really hard
> but it wouldn't remove the if (ww_ctx) branches unless I had that extra
> const bool argument.
>
I think ww_mutex was merged in 2013. That is almost 8 years ago. It
could still be the case that older gcc compilers may not generate the
right code. I will try the RHEL7 gcc compiler (4.8.5) to see how it fares.
Cheers,
Longman
On Wed, Mar 17, 2021 at 10:10:16AM -0400, Waiman Long wrote:
> On 3/17/21 9:55 AM, Peter Zijlstra wrote:
> > On Wed, Mar 17, 2021 at 09:43:20AM -0400, Waiman Long wrote:
> >
> > > Using gcc 8.4.1, the generated __mutex_lock function has the same size (with
> > > last instruction at offset +5179) with or without this patch. Well, you can
> > > say that this patch is an no-op wrt generated code.
> > OK, then GCC has gotten better. Because back then I tried really hard
> > but it wouldn't remove the if (ww_ctx) branches unless I had that extra
> > const bool argument.
> >
> I think ww_mutex was merged in 2013. That is almost 8 years ago. It could
> still be the case that older gcc compilers may not generate the right code.
> I will try the RHEL7 gcc compiler (4.8.5) to see how it fares.
I really don't care about code generation qualitee of anything before
8-ish at this point. That's already an old compiler.
If you run on ancient compilers, you simply don't care about code
quality.