* [PATCH 1/2] lkdtm: use atomic_t to replace count_lock
@ 2012-02-01 6:58 Cong Wang
2012-02-01 6:58 ` [PATCH 2/2] lkdtm: avoid calling sleeping functions in interrupt context Cong Wang
2012-02-01 15:27 ` [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Arnd Bergmann
0 siblings, 2 replies; 7+ messages in thread
From: Cong Wang @ 2012-02-01 6:58 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, Cong Wang, Prarit Bhargava, Arnd Bergmann,
Greg Kroah-Hartman, Dave Young
Andrew, this patch replaces
lkdtm-avoid-calling-lkdtm_do_action-with-spin-lock-held.patch
in your tree.
---------->
The spin lock count_lock only protects count, it can be removed by
using atomic_t. Suggested by Arnd.
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
drivers/misc/lkdtm.c | 19 ++++---------------
1 files changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 150cd70..afdef2e 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -119,8 +119,7 @@ static int recur_count = REC_NUM_DEFAULT;
static enum cname cpoint = CN_INVALID;
static enum ctype cptype = CT_NONE;
-static int count = DEFAULT_COUNT;
-static DEFINE_SPINLOCK(count_lock);
+static atomic_t count = ATOMIC_INIT(DEFAULT_COUNT);
module_param(recur_count, int, 0644);
MODULE_PARM_DESC(recur_count, " Recursion level for the stack overflow test, "\
@@ -231,14 +230,11 @@ static const char *cp_name_to_str(enum cname name)
static int lkdtm_parse_commandline(void)
{
int i;
- unsigned long flags;
if (cpoint_count < 1 || recur_count < 1)
return -EINVAL;
- spin_lock_irqsave(&count_lock, flags);
- count = cpoint_count;
- spin_unlock_irqrestore(&count_lock, flags);
+ atomic_set(&count, cpoint_count);
/* No special parameters */
if (!cpoint_type && !cpoint_name)
@@ -353,18 +349,11 @@ static void lkdtm_do_action(enum ctype which)
static void lkdtm_handler(void)
{
- unsigned long flags;
-
- spin_lock_irqsave(&count_lock, flags);
- count--;
printk(KERN_INFO "lkdtm: Crash point %s of type %s hit, trigger in %d rounds\n",
- cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
+ cp_name_to_str(cpoint), cp_type_to_str(cptype), atomic_dec_return(&count));
- if (count == 0) {
+ if (!atomic_cmpxchg(&count, 0, cpoint_count))
lkdtm_do_action(cptype);
- count = cpoint_count;
- }
- spin_unlock_irqrestore(&count_lock, flags);
}
static int lkdtm_register_cpoint(enum cname which)
--
1.7.7.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] lkdtm: avoid calling sleeping functions in interrupt context
2012-02-01 6:58 [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Cong Wang
@ 2012-02-01 6:58 ` Cong Wang
2012-02-01 15:27 ` [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Arnd Bergmann
1 sibling, 0 replies; 7+ messages in thread
From: Cong Wang @ 2012-02-01 6:58 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, Cong Wang, Prarit Bhargava, Arnd Bergmann,
Greg Kroah-Hartman, Dave Young
lkdtm_do_action() could be called in interrupt context,
but it also calls sleeping functions like schedule(),
kmalloc(GFP_KERNEL) etc., for such cases, avoid calling them
if we are in interrupt context.
BTW, check the return value of kmalloc().
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <greg@kroah.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
drivers/misc/lkdtm.c | 15 +++++++++++++--
1 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index afdef2e..63b23a4 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -311,22 +311,31 @@ static void lkdtm_do_action(enum ctype which)
}
case CT_OVERWRITE_ALLOCATION: {
size_t len = 1020;
- u32 *data = kmalloc(len, GFP_KERNEL);
+ u32 *data = kmalloc(len, GFP_ATOMIC);
+ if (!data)
+ break;
data[1024 / sizeof(u32)] = 0x12345678;
kfree(data);
break;
}
case CT_WRITE_AFTER_FREE: {
size_t len = 1024;
- u32 *data = kmalloc(len, GFP_KERNEL);
+ u32 *data;
+ if (in_interrupt())
+ break;
+ data = kmalloc(len, GFP_KERNEL);
+ if (!data)
+ break;
kfree(data);
schedule();
memset(data, 0x78, len);
break;
}
case CT_SOFTLOCKUP:
+ if (in_interrupt())
+ break;
preempt_disable();
for (;;)
cpu_relax();
@@ -337,6 +346,8 @@ static void lkdtm_do_action(enum ctype which)
cpu_relax();
break;
case CT_HUNG_TASK:
+ if (in_interrupt())
+ break;
set_current_state(TASK_UNINTERRUPTIBLE);
schedule();
break;
--
1.7.7.6
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock
2012-02-01 6:58 [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Cong Wang
2012-02-01 6:58 ` [PATCH 2/2] lkdtm: avoid calling sleeping functions in interrupt context Cong Wang
@ 2012-02-01 15:27 ` Arnd Bergmann
2012-02-02 13:33 ` Cong Wang
1 sibling, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2012-02-01 15:27 UTC (permalink / raw)
To: Cong Wang
Cc: linux-kernel, Andrew Morton, Prarit Bhargava, Greg Kroah-Hartman,
Dave Young
On Wednesday 01 February 2012, Cong Wang wrote:
> static void lkdtm_handler(void)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&count_lock, flags);
> - count--;
> printk(KERN_INFO "lkdtm: Crash point %s of type %s hit, trigger in %d rounds\n",
> - cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
> + cp_name_to_str(cpoint), cp_type_to_str(cptype), atomic_dec_return(&count));
>
> - if (count == 0) {
> + if (!atomic_cmpxchg(&count, 0, cpoint_count))
> lkdtm_do_action(cptype);
> - count = cpoint_count;
> - }
> - spin_unlock_irqrestore(&count_lock, flags);
> }
This use is not atomic, you could have two threads doing atomic_dec_return
at the same time, and after that the value will be -1 so the atomic_cmpxchg
does not trigger.
In order to have an atomic here, you have to use a loop around
atomic_cmpxchg, like
int old, new;
old = atomic_read(&count);
do {
new = old ? old - 1 : cpoint_count;
old = cmpxchg(&count, old, new);
} while (old != new);
I suppose you could also just keep the spinlock and move lkdtm_do_action()
outside of it?
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock
2012-02-01 15:27 ` [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Arnd Bergmann
@ 2012-02-02 13:33 ` Cong Wang
2012-02-02 13:44 ` Arnd Bergmann
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2012-02-02 13:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, Andrew Morton, Prarit Bhargava, Greg Kroah-Hartman,
Dave Young
On 02/01/2012 11:27 PM, Arnd Bergmann wrote:
> On Wednesday 01 February 2012, Cong Wang wrote:
>> static void lkdtm_handler(void)
>> {
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&count_lock, flags);
>> - count--;
>> printk(KERN_INFO "lkdtm: Crash point %s of type %s hit, trigger in %d rounds\n",
>> - cp_name_to_str(cpoint), cp_type_to_str(cptype), count);
>> + cp_name_to_str(cpoint), cp_type_to_str(cptype), atomic_dec_return(&count));
>>
>> - if (count == 0) {
>> + if (!atomic_cmpxchg(&count, 0, cpoint_count))
>> lkdtm_do_action(cptype);
>> - count = cpoint_count;
>> - }
>> - spin_unlock_irqrestore(&count_lock, flags);
>> }
>
> This use is not atomic, you could have two threads doing atomic_dec_return
> at the same time, and after that the value will be -1 so the atomic_cmpxchg
> does not trigger.
Yeah, simply combining two atomic operations is not atomic. :-/
>
> In order to have an atomic here, you have to use a loop around
> atomic_cmpxchg, like
>
>
> int old, new;
> old = atomic_read(&count);
> do {
> new = old ? old - 1 : cpoint_count;
> old = cmpxchg(&count, old, new);
> } while (old != new);
>
> I suppose you could also just keep the spinlock and move lkdtm_do_action()
> outside of it?
If we still need spinlock, I think we don't need to bother atomic_t at all.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock
2012-02-02 13:33 ` Cong Wang
@ 2012-02-02 13:44 ` Arnd Bergmann
2012-02-02 14:27 ` Cong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2012-02-02 13:44 UTC (permalink / raw)
To: Cong Wang
Cc: linux-kernel, Andrew Morton, Prarit Bhargava, Greg Kroah-Hartman,
Dave Young
On Thursday 02 February 2012, Cong Wang wrote:
> > In order to have an atomic here, you have to use a loop around
> > atomic_cmpxchg, like
> >
> >
> > int old, new;
> > old = atomic_read(&count);
> > do {
> > new = old ? old - 1 : cpoint_count;
> > old = cmpxchg(&count, old, new);
> > } while (old != new);
> >
> > I suppose you could also just keep the spinlock and move lkdtm_do_action()
> > outside of it?
>
> If we still need spinlock, I think we don't need to bother atomic_t at all.
Yes, it's one or the other: If you use the cmpxchg loop, you don't need a
spinlock and vice versa.
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock
2012-02-02 13:44 ` Arnd Bergmann
@ 2012-02-02 14:27 ` Cong Wang
2012-02-02 14:55 ` Arnd Bergmann
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2012-02-02 14:27 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, Andrew Morton, Prarit Bhargava, Greg Kroah-Hartman,
Dave Young
On 02/02/2012 09:44 PM, Arnd Bergmann wrote:
> On Thursday 02 February 2012, Cong Wang wrote:
>>> In order to have an atomic here, you have to use a loop around
>>> atomic_cmpxchg, like
>>>
>>>
>>> int old, new;
>>> old = atomic_read(&count);
>>> do {
>>> new = old ? old - 1 : cpoint_count;
>>> old = cmpxchg(&count, old, new);
>>> } while (old != new);
>>>
>>> I suppose you could also just keep the spinlock and move lkdtm_do_action()
>>> outside of it?
>>
>> If we still need spinlock, I think we don't need to bother atomic_t at all.
>
> Yes, it's one or the other: If you use the cmpxchg loop, you don't need a
> spinlock and vice versa.
>
The cmpxchg loop is for comparing and assigning to 'count', but still
there is a printk() above that needs to read 'count'. Combining these
two operations means we have to use a spinlock, correct? Because there
is a chance that another process could change 'count' in between.
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] lkdtm: use atomic_t to replace count_lock
2012-02-02 14:27 ` Cong Wang
@ 2012-02-02 14:55 ` Arnd Bergmann
0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2012-02-02 14:55 UTC (permalink / raw)
To: Cong Wang
Cc: linux-kernel, Andrew Morton, Prarit Bhargava, Greg Kroah-Hartman,
Dave Young
On Thursday 02 February 2012, Cong Wang wrote:
> On 02/02/2012 09:44 PM, Arnd Bergmann wrote:
> > On Thursday 02 February 2012, Cong Wang wrote:
> >>> In order to have an atomic here, you have to use a loop around
> >>> atomic_cmpxchg, like
> >>>
> >>>
> >>> int old, new;
> >>> old = atomic_read(&count);
> >>> do {
> >>> new = old ? old - 1 : cpoint_count;
> >>> old = cmpxchg(&count, old, new);
^^^^^^^
I guess I meant "new = cmpxchg(...)" here, sorry.
> >>> } while (old != new);
> >>>
> >>> I suppose you could also just keep the spinlock and move lkdtm_do_action()
> >>> outside of it?
> >>
> >> If we still need spinlock, I think we don't need to bother atomic_t at all.
> >
> > Yes, it's one or the other: If you use the cmpxchg loop, you don't need a
> > spinlock and vice versa.
> >
>
> The cmpxchg loop is for comparing and assigning to 'count', but still
> there is a printk() above that needs to read 'count'. Combining these
> two operations means we have to use a spinlock, correct? Because there
> is a chance that another process could change 'count' in between.
No, you can just print the value of "old" in the above example,
which was atomically read.
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-02-02 14:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-01 6:58 [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Cong Wang
2012-02-01 6:58 ` [PATCH 2/2] lkdtm: avoid calling sleeping functions in interrupt context Cong Wang
2012-02-01 15:27 ` [PATCH 1/2] lkdtm: use atomic_t to replace count_lock Arnd Bergmann
2012-02-02 13:33 ` Cong Wang
2012-02-02 13:44 ` Arnd Bergmann
2012-02-02 14:27 ` Cong Wang
2012-02-02 14:55 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).