[PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with mutex

[PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with mutex

[Chuansheng Liu]

When do code reviewing, found no special requirement to
use spin_lock_irqsave/spin_unlock_irqrestore, because
alarmtimer_get_rtcdev() is called by posix clock interface.
So would like to use mutex to replace it.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/time/alarmtimer.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index f11d83b..4fc17cb 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -51,7 +51,7 @@ static struct wakeup_source *ws;
 /* rtc timer and device for setting alarm wakeups at suspend */
 static struct rtc_timer		rtctimer;
 static struct rtc_device	*rtcdev;
-static DEFINE_SPINLOCK(rtcdev_lock);
+static DEFINE_MUTEX(rtcdev_mutex);
 
 /**
  * alarmtimer_get_rtcdev - Return selected rtcdevice
@@ -62,12 +62,11 @@ static DEFINE_SPINLOCK(rtcdev_lock);
  */
 struct rtc_device *alarmtimer_get_rtcdev(void)
 {
-	unsigned long flags;
 	struct rtc_device *ret;
 
-	spin_lock_irqsave(&rtcdev_lock, flags);
+	mutex_lock(&rtcdev_mutex);
 	ret = rtcdev;
-	spin_unlock_irqrestore(&rtcdev_lock, flags);
+	mutex_unlock(&rtcdev_mutex);
 
 	return ret;
 }
@@ -76,7 +75,6 @@ struct rtc_device *alarmtimer_get_rtcdev(void)
 static int alarmtimer_rtc_add_device(struct device *dev,
 				struct class_interface *class_intf)
 {
-	unsigned long flags;
 	struct rtc_device *rtc = to_rtc_device(dev);
 
 	if (rtcdev)
@@ -87,13 +85,13 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	if (!device_may_wakeup(rtc->dev.parent))
 		return -1;
 
-	spin_lock_irqsave(&rtcdev_lock, flags);
+	mutex_lock(&rtcdev_mutex);
 	if (!rtcdev) {
 		rtcdev = rtc;
 		/* hold a reference so it doesn't go away */
 		get_device(dev);
 	}
-	spin_unlock_irqrestore(&rtcdev_lock, flags);
+	mutex_unlock(&rtcdev_mutex);
 	return 0;
 }
 
-- 
1.7.0.4

Re: [PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with mutex

[Oliver Neukum]

On Thursday 01 November 2012 00:20:55 Chuansheng Liu wrote:
> When do code reviewing, found no special requirement to
> use spin_lock_irqsave/spin_unlock_irqrestore, because
> alarmtimer_get_rtcdev() is called by posix clock interface.
> So would like to use mutex to replace it.

What is gained thereby?

	Regards
		Oliver

RE: [PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with mutex

[Liu, Chuansheng]

> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.de]
> Sent: Wednesday, October 31, 2012 5:03 PM
> To: Liu, Chuansheng
> Cc: john.stultz@linaro.org; tglx@linutronix.de; gregkh@linuxfoundation.org;
> linux-kernel@vger.kernel.org; Liu, Chuansheng
> Subject: Re: [PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with
> mutex
> 
> On Thursday 01 November 2012 00:20:55 Chuansheng Liu wrote:
> > When do code reviewing, found no special requirement to
> > use spin_lock_irqsave/spin_unlock_irqrestore, because
> > alarmtimer_get_rtcdev() is called by posix clock interface.
> > So would like to use mutex to replace it.
> 
> What is gained thereby?
spin_lock_irqsave will disable the preempt and local irq, it is expensive than
mutex. Thanks.
> 
> 	Regards
> 		Oliver

RE: [PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with mutex

[Thomas Gleixner]

On Thu, 1 Nov 2012, Liu, Chuansheng wrote:
> > From: Oliver Neukum [mailto:oneukum@suse.de]
> > On Thursday 01 November 2012 00:20:55 Chuansheng Liu wrote:
> > > When do code reviewing, found no special requirement to
> > > use spin_lock_irqsave/spin_unlock_irqrestore, because
> > > alarmtimer_get_rtcdev() is called by posix clock interface.
> > > So would like to use mutex to replace it.
> > 
> > What is gained thereby?
> spin_lock_irqsave will disable the preempt and local irq, it is expensive than
> mutex. Thanks.

Let's have a look at how expensive that is. The function which is
relevant is alarmtimer_get_rtcdev(), which does:

        spin_lock_irqsave(&rtcdev_lock, flags);
        ret = rtcdev;
        spin_unlock_irqrestore(&rtcdev_lock, flags);
        return ret;

So now you make rtcdev_lock a mutex, which means that for a single
protected instruction i.e "ret = rtcdev" you want to use a
serialization mechanism which is way more expensive when it actually
comes to a contention.

Now we could debate this back and forth, but this is pointless as your
code review missed the real issue:

 "Protecting" the dereferencing of rtcdev with any kind of lock does
 not provide any useful protection at all.

Lets look at the code which manipulates rtcdev.

The only function which does that is alarmtimer_rtc_add_device(). This
function only can set the pointer ONCE. Now that function needs a
serialization and again we can debate whether this needs to be a
spinlock or a mutex. So now that we know that a device which has been
registered with that function can never go away and is never going to
change, it's completely pointless to have a lock in
alarmtimer_get_rtcdev() at all.

It's safe to do in any code path which wants to use rtcdev:

     if (!rtcdev)
     	return -ENODEV:
     do_something(rtcdev);

Thanks,

	tglx

RE: [PATCH 1/3] alarmtimer: Replace the spinlock rtcdev_lock with mutex

[Liu, Chuansheng]

> It's safe to do in any code path which wants to use rtcdev:
> 
>      if (!rtcdev)
>      	return -ENODEV:
>      do_something(rtcdev);
> 
Really thanks your analysis and sharing, learn something.
And preparing a new patch with your comments for this point.

Subject: [PATCH] alarmtimer: Removing lock at alarmtimer_get_rtcdev()

[From Thomas comments]
"Protecting" the dereferencing of rtcdev with any kind of lock does
not provide any useful protection at all.

Moreover, alarmtimer_rtc_add_device just set the rtc device ONCE,
and the device will never go away and is never going to change, it's
completely pointless to have a lock in alarmtimer_get_rtcdev() at all.

So this patch remove the lock/unlock actions in alarmtimer_get_rtcdev().

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/time/alarmtimer.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index f11d83b..eed3b26 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -62,14 +62,7 @@ static DEFINE_SPINLOCK(rtcdev_lock);
  */
 struct rtc_device *alarmtimer_get_rtcdev(void)
 {
-       unsigned long flags;
-       struct rtc_device *ret;
-
-       spin_lock_irqsave(&rtcdev_lock, flags);
-       ret = rtcdev;
-       spin_unlock_irqrestore(&rtcdev_lock, flags);
-
-       return ret;
+       return rtcdev;
 }
 
 
@@ -224,10 +217,10 @@ static int alarmtimer_suspend(struct device *dev)
        freezer_delta = ktime_set(0, 0);
        spin_unlock_irqrestore(&freezer_delta_lock, flags);
 
-       rtc = alarmtimer_get_rtcdev();
        /* If we have no rtcdev, just return */
-       if (!rtc)
+       if (!rtcdev)
                return 0;
+       rtc = rtcdev;
 
        /* Find the soonest timer to expire*/
        for (i = 0; i < ALARM_NUMTYPE; i++) {
@@ -444,7 +437,7 @@ static int alarm_clock_getres(const clockid_t which_clock, struct timespec *tp)
 {
        clockid_t baseid = alarm_bases[clock2alarm(which_clock)].base_clockid;
 
-       if (!alarmtimer_get_rtcdev())
+       if (!rtcdev)
                return -ENOTSUPP;
 
        return hrtimer_get_res(baseid, tp);
@@ -461,7 +454,7 @@ static int alarm_clock_get(clockid_t which_clock, struct timespec *tp)
 {
        struct alarm_base *base = &alarm_bases[clock2alarm(which_clock)];
 
-       if (!alarmtimer_get_rtcdev())
+       if (!rtcdev)
                return -ENOTSUPP;
 
        *tp = ktime_to_timespec(base->gettime());
@@ -479,7 +472,7 @@ static int alarm_timer_create(struct k_itimer *new_timer)
        enum  alarmtimer_type type;
        struct alarm_base *base;
 
-       if (!alarmtimer_get_rtcdev())
+       if (!rtcdev)
                return -ENOTSUPP;
 
        if (!capable(CAP_WAKE_ALARM))
@@ -682,7 +675,7 @@ static int alarm_timer_nsleep(const clockid_t which_clock, int flags,
        int ret = 0;
        struct restart_block *restart;
 
-       if (!alarmtimer_get_rtcdev())
+       if (!rtcdev)
                return -ENOTSUPP;
 
        if (!capable(CAP_WAKE_ALARM))
-- 
1.7.0.4

[PATCH 2/3] alarmtimer: Using the alarmtimer_get_rtcdev for all posix clock interface

[Chuansheng Liu]

Some posix clock interface directly use the variable rtcdev,
cleanup it here by alarmtimer_get_rtcdev().

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/time/alarmtimer.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 4fc17cb..5490fa8 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -86,11 +86,10 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 		return -1;
 
 	mutex_lock(&rtcdev_mutex);
-	if (!rtcdev) {
-		rtcdev = rtc;
-		/* hold a reference so it doesn't go away */
-		get_device(dev);
-	}
+	rtcdev = rtc;
+	/* hold a reference so it doesn't go away */
+	get_device(dev);
+
 	mutex_unlock(&rtcdev_mutex);
 	return 0;
 }
@@ -516,7 +515,7 @@ static void alarm_timer_get(struct k_itimer *timr,
  */
 static int alarm_timer_del(struct k_itimer *timr)
 {
-	if (!rtcdev)
+	if (!alarmtimer_get_rtcdev())
 		return -ENOTSUPP;
 
 	if (alarm_try_to_cancel(&timr->it.alarm.alarmtimer) < 0)
@@ -538,7 +537,7 @@ static int alarm_timer_set(struct k_itimer *timr, int flags,
 				struct itimerspec *new_setting,
 				struct itimerspec *old_setting)
 {
-	if (!rtcdev)
+	if (!alarmtimer_get_rtcdev())
 		return -ENOTSUPP;
 
 	if (old_setting)
-- 
1.7.0.4

Re: [PATCH 2/3] alarmtimer: Using the alarmtimer_get_rtcdev for all posix clock interface

[Thomas Gleixner]

On Thu, 1 Nov 2012, Chuansheng Liu wrote:
> 
> Some posix clock interface directly use the variable rtcdev,
> cleanup it here by alarmtimer_get_rtcdev().
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  kernel/time/alarmtimer.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 4fc17cb..5490fa8 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -86,11 +86,10 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>  		return -1;
>  
>  	mutex_lock(&rtcdev_mutex);
> -	if (!rtcdev) {
> -		rtcdev = rtc;
> -		/* hold a reference so it doesn't go away */
> -		get_device(dev);
> -	}
> +	rtcdev = rtc;
> +	/* hold a reference so it doesn't go away */
> +	get_device(dev);
> +

Brilliant. 

rtcdev = NULL

CPU0				CPU 1

alarmtimer_rtc_add_device(A)
	if (rtcdev)		alarmtimer_rtc_add_device(B)
	     return -EBUSY;		if (rtcdev)
	mutex_lock();				return -EBUSY;
	rtcdev = A;			mutex_lock();	
	mutex_unlock;

bla = alarmtimer_rtc_get_device()

So bla = A
      				  	mutex_lock() returns
      				  	rtcdev = B;
					mutex_unlock();

The next call to alarmtimer_rtc_get_device() will return B. Not what
you want. Maybe you want that, but then your patch is missing an
explanation why you want that and why this would be a desired
behaviour.

Thanks,

	tglx

RE: [PATCH 2/3] alarmtimer: Using the alarmtimer_get_rtcdev for all posix clock interface

[Liu, Chuansheng]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Friday, November 02, 2012 6:56 AM
> To: Liu, Chuansheng
> Cc: john.stultz@linaro.org; gregkh@linuxfoundation.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] alarmtimer: Using the alarmtimer_get_rtcdev for all
> posix clock interface
> 
> On Thu, 1 Nov 2012, Chuansheng Liu wrote:
> >
> > Some posix clock interface directly use the variable rtcdev,
> > cleanup it here by alarmtimer_get_rtcdev().
> >
> > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> > ---
> >  kernel/time/alarmtimer.c |   13 ++++++-------
> >  1 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> > index 4fc17cb..5490fa8 100644
> > --- a/kernel/time/alarmtimer.c
> > +++ b/kernel/time/alarmtimer.c
> > @@ -86,11 +86,10 @@ static int alarmtimer_rtc_add_device(struct device
> *dev,
> >  		return -1;
> >
> >  	mutex_lock(&rtcdev_mutex);
> > -	if (!rtcdev) {
> > -		rtcdev = rtc;
> > -		/* hold a reference so it doesn't go away */
> > -		get_device(dev);
> > -	}
> > +	rtcdev = rtc;
> > +	/* hold a reference so it doesn't go away */
> > +	get_device(dev);
> > +
> 
> Brilliant.
> 
> rtcdev = NULL
> 
> CPU0				CPU 1
> 
> alarmtimer_rtc_add_device(A)
> 	if (rtcdev)		alarmtimer_rtc_add_device(B)
> 	     return -EBUSY;		if (rtcdev)
> 	mutex_lock();				return -EBUSY;
> 	rtcdev = A;			mutex_lock();
> 	mutex_unlock;
> 
> bla = alarmtimer_rtc_get_device()
> 
> So bla = A
>       				  	mutex_lock() returns
>       				  	rtcdev = B;
> 					mutex_unlock();
> 
> The next call to alarmtimer_rtc_get_device() will return B. Not what
> you want. Maybe you want that, but then your patch is missing an
> explanation why you want that and why this would be a desired
> behaviour.
Thanks your pointing out, I am wrong.
> 
> Thanks,
> 
> 	tglx

RE: [PATCH 2/3] alarmtimer: Using the alarmtimer_get_rtcdev for all posix clock interface

[Liu, Chuansheng]

> rtcdev = NULL
> 
> CPU0				CPU 1
> 
> alarmtimer_rtc_add_device(A)
> 	if (rtcdev)		alarmtimer_rtc_add_device(B)
> 	     return -EBUSY;		if (rtcdev)
> 	mutex_lock();				return -EBUSY;
> 	rtcdev = A;			mutex_lock();
> 	mutex_unlock;
> 
> bla = alarmtimer_rtc_get_device()
> 
> So bla = A
>       				  	mutex_lock() returns
>       				  	rtcdev = B;
> 					mutex_unlock();
> 
> The next call to alarmtimer_rtc_get_device() will return B. Not what
> you want. Maybe you want that, but then your patch is missing an
> explanation why you want that and why this would be a desired
> behaviour.
> 

Based on current implementation alarmtimer_rtc_add_device() will just
be called once thru class interface, also &parent->p->mutex is there to protect
the class_intf->add_dev() calling, so we can remove rtcdev_lock.
Is it making sense?


Subject: [PATCH] alarmtimer: Removing the usage of lock rtcdev_lock

alarmtimer_rtc_add_device() will be called once at the time
alarmtimer_init() is called.

alarmtimer_init -->
        alarmtimer_rtc_interface_setup -->
                class_interface_register -->
                        mutex_lock(&parent->p->mutex)
                        ...
                        class_intf->add_dev()/alarmtimer_rtc_add_device
                        ...
                        mutex_unlock(&parent->p->mutex);

But also the mutex parent->p->mutex has protected the add_dev()
callback.

So here removing the lock of rtcdev_lock.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/time/alarmtimer.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index eed3b26..660091c 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -51,7 +51,6 @@ static struct wakeup_source *ws;
 /* rtc timer and device for setting alarm wakeups at suspend */
 static struct rtc_timer                rtctimer;
 static struct rtc_device       *rtcdev;
-static DEFINE_SPINLOCK(rtcdev_lock);
 
 /**
  * alarmtimer_get_rtcdev - Return selected rtcdevice
@@ -69,7 +68,6 @@ struct rtc_device *alarmtimer_get_rtcdev(void)
 static int alarmtimer_rtc_add_device(struct device *dev,
                                struct class_interface *class_intf)
 {
-       unsigned long flags;
        struct rtc_device *rtc = to_rtc_device(dev);
 
        if (rtcdev)
@@ -80,13 +78,10 @@ static int alarmtimer_rtc_add_device(struct device *dev,
        if (!device_may_wakeup(rtc->dev.parent))
                return -1;
 
-       spin_lock_irqsave(&rtcdev_lock, flags);
-       if (!rtcdev) {
-               rtcdev = rtc;
-               /* hold a reference so it doesn't go away */
-               get_device(dev);
-       }
-       spin_unlock_irqrestore(&rtcdev_lock, flags);
+       rtcdev = rtc;
+       /* hold a reference so it doesn't go away */
+       get_device(dev);
+
        return 0;
 }
 
-- 
1.7.0.4

[PATCH 3/3] alarmtimer: cleanup the POSIX clock interface without CONFIG_RTC_CLASS

[Chuansheng Liu]

When CONFIG_RTC_CLASS is not defined, implementing the POSIX clock
interface with null function is enough.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 kernel/time/alarmtimer.c |   78 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 5490fa8..12c17a4 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -41,10 +41,6 @@ static struct alarm_base {
 	clockid_t		base_clockid;
 } alarm_bases[ALARM_NUMTYPE];
 
-/* freezer delta & lock used to handle clock_nanosleep triggered wakeups */
-static ktime_t freezer_delta;
-static DEFINE_SPINLOCK(freezer_delta_lock);
-
 static struct wakeup_source *ws;
 
 #ifdef CONFIG_RTC_CLASS
@@ -53,6 +49,10 @@ static struct rtc_timer		rtctimer;
 static struct rtc_device	*rtcdev;
 static DEFINE_MUTEX(rtcdev_mutex);
 
+/* freezer delta & lock used to handle clock_nanosleep triggered wakeups */
+static ktime_t freezer_delta;
+static DEFINE_SPINLOCK(freezer_delta_lock);
+
 /**
  * alarmtimer_get_rtcdev - Return selected rtcdevice
  *
@@ -268,21 +268,6 @@ static int alarmtimer_suspend(struct device *dev)
 }
 #endif
 
-static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
-{
-	ktime_t delta;
-	unsigned long flags;
-	struct alarm_base *base = &alarm_bases[type];
-
-	delta = ktime_sub(absexp, base->gettime());
-
-	spin_lock_irqsave(&freezer_delta_lock, flags);
-	if (!freezer_delta.tv64 || (delta.tv64 < freezer_delta.tv64))
-		freezer_delta = delta;
-	spin_unlock_irqrestore(&freezer_delta_lock, flags);
-}
-
-
 /**
  * alarm_init - Initialize an alarm structure
  * @alarm: ptr to alarm to be initialized
@@ -393,6 +378,21 @@ u64 alarm_forward(struct alarm *alarm, ktime_t now, ktime_t interval)
 
 
 
+#ifdef CONFIG_RTC_CLASS
+static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
+{
+	ktime_t delta;
+	unsigned long flags;
+	struct alarm_base *base = &alarm_bases[type];
+
+	delta = ktime_sub(absexp, base->gettime());
+
+	spin_lock_irqsave(&freezer_delta_lock, flags);
+	if (!freezer_delta.tv64 || (delta.tv64 < freezer_delta.tv64))
+		freezer_delta = delta;
+	spin_unlock_irqrestore(&freezer_delta_lock, flags);
+}
+
 
 /**
  * clock2alarm - helper that converts from clockid to alarmtypes
@@ -722,7 +722,47 @@ static int alarm_timer_nsleep(const clockid_t which_clock, int flags,
 out:
 	return ret;
 }
+#else
+static inline int alarm_clock_getres(const clockid_t which_clock,
+						struct timespec *tp)
+{
+	return 0;
+}
+
+static inline int alarm_clock_get(clockid_t which_clock, struct timespec *tp)
+{
+	return 0;
+}
 
+static inline int alarm_timer_create(struct k_itimer *new_timer)
+{
+	return 0;
+}
+
+static inline int alarm_timer_set(struct k_itimer *timr, int flags,
+				struct itimerspec *new_setting,
+				struct itimerspec *old_setting)
+{
+	return 0;
+}
+
+static inline int alarm_timer_del(struct k_itimer *timr)
+{
+	return 0;
+}
+
+static inline void alarm_timer_get(struct k_itimer *timr,
+				struct itimerspec *cur_setting)
+{
+	return;
+}
+
+static inline int alarm_timer_nsleep(const clockid_t which_clock, int flags,
+		     struct timespec *tsreq, struct timespec __user *rmtp)
+{
+	return 0;
+}
+#endif
 
 /* Suspend hook structures */
 static const struct dev_pm_ops alarmtimer_pm_ops = {
-- 
1.7.0.4