From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932476AbbFEJKg (ORCPT ); Fri, 5 Jun 2015 05:10:36 -0400 Received: from casper.infradead.org ([85.118.1.10]:51725 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932222AbbFEJKc (ORCPT ); Fri, 5 Jun 2015 05:10:32 -0400 Date: Fri, 5 Jun 2015 11:10:27 +0200 From: Peter Zijlstra To: Kirill Tkhai Cc: "umgwanakikbuti@gmail.com" , "mingo@elte.hu" , "ktkhai@parallels.com" , "rostedt@goodmis.org" , "tglx@linutronix.de" , "juri.lelli@gmail.com" , "pang.xunlei@linaro.org" , "oleg@redhat.com" , "wanpeng.li@linux.intel.com" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 8/9] hrtimer: Allow hrtimer::function() to free the timer Message-ID: <20150605091027.GE19282@twins.programming.kicks-ass.net> References: <20150603132903.203333087@infradead.org> <20150603134023.156059118@infradead.org> <214021433348760@web25g.yandex.ru> <20150603211324.GC3644@twins.programming.kicks-ass.net> <2134411433408823@web8j.yandex.ru> <20150604104902.GH3644@twins.programming.kicks-ass.net> <5883311433494921@web27h.yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5883311433494921@web27h.yandex.ru> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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.