lkml.org 
[lkml]   [2020]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH RT 10/30] hrtimer: Prevent using hrtimer_grab_expiry_lock() on migration_base
    From
    Date
    On Tue, 2020-04-28 at 15:07 +0200, Rasmus Villemoes wrote:
    > On 28/04/2020 14.59, Tom Zanussi wrote:
    > > On Tue, 2020-04-28 at 09:03 +0200, Rasmus Villemoes wrote:
    > > > Hold on a second. This patch (hrtimer: Prevent using
    > > > hrtimer_grab_expiry_lock() on migration_base) indeed seems to
    > > > implement
    > > > the optimization implied by the above, namely avoid the
    > > > lock/unlock
    > > > in
    > > > case base == migration_base:
    > > >
    > > > > - if (timer->is_soft && base && base->cpu_base) {
    > > > > + if (timer->is_soft && base != &migration_base) {
    > > >
    > > > But the followup patch (hrtimer: Add a missing bracket and hide
    > > > `migration_base on !SMP) to fix the build on !SMP [the missing
    > > > bracket
    > > > part seems to have been fixed when backporting the above to 4.19-
    > > > rt]
    > > > replaces that logic by
    > > >
    > > > +static inline bool is_migration_base(struct hrtimer_clock_base
    > > > *base)
    > > > +{
    > > > + return base == &migration_base;
    > > > +}
    > > > +
    > > > ...
    > > > - if (timer->is_soft && base != &migration_base) {
    > > > + if (timer->is_soft && is_migration_base(base)) {
    > > >
    > > > in the SMP case, i.e. the exact opposite condition. One of these
    > > > can't
    > > > be correct.
    > > >
    > > > Assuming the followup patch was wrong and the condition should
    > > > have
    > > > read
    > > >
    > > > timer->is_soft && !is_migration_base(base)
    > > >
    > > > while keeping is_migration_base() false on !SMP might explain the
    > > > problem I see. But I'd like someone who knows this code to chime
    > > > in.
    > > >
    > >
    > > I don't know this code, but I think you're correct - the followup
    > > patch
    > > reversed the condition by forgetting the !.
    > >
    > > So, does your problem go away when you make that change?
    >
    > Yes, it does. (I'll have to ask the customer to check in their setup
    > whether the boot hang also vanishes).
    >
    > Essentially, adding that ! is equivalent to reverting the two patches
    > on
    > !SMP (which I also tested): Before, the condition was
    >
    > timer->is_soft && base && base->cpu_base
    >
    > and, assuming the NULL pointer checks are indeed redundant, that's
    > the
    > same as "timer->is_soft". Appending " && !is_migration_base()" to
    > that,
    > with is_migration_base() always false as on !SMP, doesn't change
    > anything.
    >

    OK, great, thanks for tracking this down.

    If you post a patch that makes that change and mention that it's a fix
    for commit "40aae5708e7a hrtimer: Add a missing bracket and hide
    `migration_base' on !SMP", I can pull it into a new update release.

    Thanks,

    Tom

    > Thanks,
    > Rasmus

    \
     
     \ /
      Last update: 2020-04-28 15:43    [W:7.868 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site