linux-kernel - Re: [PATCH 8/9] hrtimer: Allow hrtimer::function() to free the timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <6034121433496470@web27h.yandex.ru>
Date:	Fri, 05 Jun 2015 12:27:50 +0300
From:	Kirill Tkhai <tkhai@...dex.ru>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	"umgwanakikbuti@...il.com" <umgwanakikbuti@...il.com>,
	"mingo@...e.hu" <mingo@...e.hu>,
	"ktkhai@...allels.com" <ktkhai@...allels.com>,
	"rostedt@...dmis.org" <rostedt@...dmis.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"juri.lelli@...il.com" <juri.lelli@...il.com>,
	"pang.xunlei@...aro.org" <pang.xunlei@...aro.org>,
	"oleg@...hat.com" <oleg@...hat.com>,
	"wanpeng.li@...ux.intel.com" <wanpeng.li@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 8/9] hrtimer: Allow hrtimer::function() to free the timer

05.06.2015, 12:10, "Peter Zijlstra" <peterz@...radead.org>:
> On Fri, Jun 05, 2015 at 12:02:01PM +0300, Kirill Tkhai wrote:
>>  Yeah, it's safe for now, but it may happen difficulties with a support
>>  in the future, because barrier logic is not easy to review. But it seems
>>  we may simplify it a little bit. Please, see the comments below.
>>>  @@ -394,8 +399,24 @@ extern u64 hrtimer_get_next_event(void);
>>>    */
>>>   static inline int hrtimer_active(const struct hrtimer *timer)
>>>   {
>>>  + struct hrtimer_cpu_base *cpu_base;
>>>  + unsigned int seq;
>>>  + bool active;
>>>  +
>>>  + do {
>>>  + active = false;
>>>  + cpu_base = READ_ONCE(timer->base->cpu_base);
>>>  + seqcount_lockdep_reader_access(&cpu_base->seq);
>>>  + seq = raw_read_seqcount(&cpu_base->seq);
>>>  +
>>>  + if (timer->state != HRTIMER_STATE_INACTIVE ||
>>>  +    cpu_base->running == timer)
>>>  + active = true;
>>>  +
>>>  + } while (read_seqcount_retry(&cpu_base->seq, seq) ||
>>>  + cpu_base != READ_ONCE(timer->base->cpu_base));
>>>  +
>>>  + return active;
>>>   }
>>  This may race with migrate_hrtimer_list(), so it needs write seqcounter
>>  too.
>
> Let me go stare at that.
>>  My suggestion is do not use seqcounters for long parts of code, and implement
>>  short primitives for changing timer state and cpu_base running timer. Something
>>  like this:
>>
>>  static inline void hrtimer_set_state(struct hrtimer *timer, unsigned long state)
>>  {
>>          struct hrtimer_cpu_base *cpu_base = timer->base->cpu_base;
>>
>>          lockdep_assert_held(&cpu_base->lock);
>>
>>          write_seqcount_begin(&cpu_base->seq);
>>          timer->state = state;
>>          write_seqcount_end(&cpu_base->seq);
>>  }
>>
>>  static inline void cpu_base_set_running(struct hrtimer_cpu_base *cpu_base,
>>                                          struct hrtimer *timer)
>>  {
>>          lockdep_assert_held(&cpu_base->lock);
>>
>>          write_seqcount_begin(&cpu_base->seq);
>>          cpu_base->running = timer;
>>          write_seqcount_end(&cpu_base->seq);
>>  }
>>
>>  Implemented this, we may less think about right barrier order, because
>>  all changes are being made under seqcount.
>
> The problem with that is that it generates far more memory barriers,
> while on x86 these are 'free', this is very much not so for other archs
> like ARM / PPC etc.

Ok, thanks.

One more way is to take write_seqcount every time we're taking base spin_lock,
thus we may group several smp_wmb() in a single. But this increases write
seqlocked code areas, and worsens the speed of hrtimer_active(), so it's not
good too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/