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 <[email protected]>
---
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
Some posix clock interface directly use the variable rtcdev,
cleanup it here by alarmtimer_get_rtcdev().
Signed-off-by: liu chuansheng <[email protected]>
---
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
When CONFIG_RTC_CLASS is not defined, implementing the POSIX clock
interface with null function is enough.
Signed-off-by: liu chuansheng <[email protected]>
---
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
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
> -----Original Message-----
> From: Oliver Neukum [mailto:[email protected]]
> Sent: Wednesday, October 31, 2012 5:03 PM
> To: Liu, Chuansheng
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; 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
On Thu, 1 Nov 2012, Liu, Chuansheng wrote:
> > From: Oliver Neukum [mailto:[email protected]]
> > 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
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 <[email protected]>
> ---
> 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
> 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 <[email protected]>
---
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
> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: Friday, November 02, 2012 6:56 AM
> To: Liu, Chuansheng
> Cc: [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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
> 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 <[email protected]>
---
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