cgroup: make cgrp->event_list_lock irqsafe

PATCH In review Li Zefan 2 participants 7 messages Mar 6, 2013 cgroup

Share & tools

Li Zefan

author

2013-03-06 03:28 UTC #0 lore raw

PATCH 1/1 cgroup: make cgrp->event_list_lock irqsafe

cgroup_event_wake() is called with hardirq-safe wqh->lock held, so the nested cgrp->event_list_lock should also be hardirq-safe.

Fortunately I don't think the deadlock can happen in real life.

Lockdep never complained, maybe because it never found wqh->lock was held in irq context?

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

kernel/cgroup.c +9 -8

index 7a6c4c7..e72c44e 100644
@@ -3845,9 +3845,10 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 	struct cgroup_event *event = container_of(wait,
 			struct cgroup_event, wait);
 	struct cgroup *cgrp = event->cgrp;
-	unsigned long flags = (unsigned long)key;
+	unsigned long poll_flags = (unsigned long)key;
+	unsigned long flags;
 
-	if (flags & POLLHUP) {
+	if (poll_flags & POLLHUP) {
 		/*
 		 * If the event has been detached at cgroup removal, we
 		 * can simply return knowing the other side will cleanup

@@ -3857,7 +3858,7 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 		 * side will require wqh->lock via remove_wait_queue(),
 		 * which we hold.
 		 */
-		spin_lock(&cgrp->event_list_lock);
+		spin_lock_irqsave(&cgrp->event_list_lock, flags);
 		if (!list_empty(&event->list)) {
 			list_del_init(&event->list);
 			/*

@@ -3866,7 +3867,7 @@ static int cgroup_event_wake(wait_queue_t *wait, unsigned mode,
 			 */
 			schedule_work(&event->remove);
 		}
-		spin_unlock(&cgrp->event_list_lock);
+		spin_unlock_irqrestore(&cgrp->event_list_lock, flags);
 	}
 
 	return 0;

@@ -3981,9 +3982,9 @@ static int cgroup_write_event_control(struct cgroup *cgrp, struct cftype *cft,
 	 */
 	dget(cgrp->dentry);
 
-	spin_lock(&cgrp->event_list_lock);
+	spin_lock_irq(&cgrp->event_list_lock);
 	list_add(&event->list, &cgrp->event_list);
-	spin_unlock(&cgrp->event_list_lock);
+	spin_unlock_irq(&cgrp->event_list_lock);
 
 	fput(cfile);
 	fput(efile);

@@ -4406,12 +4407,12 @@ static int cgroup_destroy_locked(struct cgroup *cgrp)
 	 * Notify userspace about cgroup removing only after rmdir of cgroup
 	 * directory to avoid race between userspace and kernelspace.
 	 */
-	spin_lock(&cgrp->event_list_lock);
+	spin_lock_irq(&cgrp->event_list_lock);
 	list_for_each_entry_safe(event, tmp, &cgrp->event_list, list) {
 		list_del_init(&event->list);
 		schedule_work(&event->remove);
 	}
-	spin_unlock(&cgrp->event_list_lock);
+	spin_unlock_irq(&cgrp->event_list_lock);
 
 	return 0;
 }

Signature · 1 line

1.8.0.2

Tejun Heo

maintainer

2013-03-06 06:22 UTC #1 lore raw

Hello, Li.

Quoted 7 lines · Li Zefan

cgroup_event_wake() is called with hardirq-safe wqh->lock held, so the nested cgrp->event_list_lock should also be hardirq-safe.
Fortunately I don't think the deadlock can happen in real life.
Lockdep never complained, maybe because it never found wqh->lock was held in irq context?

Why should wqh->lock be hard-irq-safe? Is it actually grabbed from irq context? Locks which are grabbed with irq disabled aren't necessarily irq context locks as that doesn't lead to deadlocks. They need to be actually grabbed from irq context.

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2013-03-06 07:00 UTC #2 lore raw

Quoted 6 lines · Tejun Heo

Hello, Li.
Quoted 3 lines · Li Zefan
cgroup_event_wake() is called with hardirq-safe wqh->lock held, so the nested cgrp->event_list_lock should also be hardirq-safe.

Quoted 7 lines

Quoted 4 lines
Fortunately I don't think the deadlock can happen in real life.
Lockdep never complained, maybe because it never found wqh->lock was held in irq context?
Why should wqh->lock be hard-irq-safe? Is it actually grabbed from irq context?

becase cgroup_event_wake() is a callback to a wait queue, and it's wake_up() that acquires wqh->lock with irq disabled.

Quoted 6 lines

Locks which are grabbed with irq disabled aren't necessarily irq context locks as that doesn't lead to deadlocks. They need to be actually grabbed from irq context.
Thanks.

Tejun Heo

maintainer

2013-03-06 07:02 UTC #3 lore raw

Quoted 5 lines · Li Zefan

Quoted 2 lines
Why should wqh->lock be hard-irq-safe? Is it actually grabbed from irq context?
becase cgroup_event_wake() is a callback to a wait queue, and it's wake_up() that acquires wqh->lock with irq disabled.

So, acquiring a lock with irq disabled doesn't make it a irq lock. Being grabbed *from* irq handler makes it a irq lock. Would the wake_up() happen from irq handler?

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2013-03-06 07:15 UTC #4 lore raw

Quoted 11 lines · Tejun Heo

Quoted 5 lines · Li Zefan
Quoted 2 lines
Why should wqh->lock be hard-irq-safe? Is it actually grabbed from irq context?
becase cgroup_event_wake() is a callback to a wait queue, and it's wake_up() that acquires wqh->lock with irq disabled.
So, acquiring a lock with irq disabled doesn't make it a irq lock. Being grabbed *from* irq handler makes it a irq lock. Would the wake_up() happen from irq handler?

wqh->lock is used through out fs/eventfd.c. I don't know if currently there's any kernel user using eventfd APIs in an irq handler, but at least that should be allowed.

wake_up() is also allowed to be called from irq handler?

"allowed" should be enough reason we forbid:

	spin_lock_irqsave(...)
	spin_lock(...)

Tejun Heo

maintainer

2013-03-06 07:21 UTC #5 lore raw

Hello, Li.

Quoted 5 lines · Li Zefan

wqh->lock is used through out fs/eventfd.c. I don't know if currently there's any kernel user using eventfd APIs in an irq handler, but at least that should be allowed.
wake_up() is also allowed to be called from irq handler?

Yeah, well, we don't consider everything which uses wake_up() to be irq-safe. I'm not necessarily against making the lock irq-safe but at least the commit message is misleading as currently posted. I'd suggest looking at eventfd and see whether the wakeup could actually happen from irq context. If not, I don't know, I don't think it matters either way. It's not like it's a difficult one to detect with lockdep.

Signature · 1 line

tejun

Tejun Heo

maintainer

2013-03-06 07:23 UTC #6 lore raw

Quoted 4 lines · Tejun Heo

Quoted 1 line
wake_up() is also allowed to be called from irq handler?
Yeah, well, we don't consider everything which uses wake_up() to be irq-safe. I'm not necessarily against making the lock irq-safe but at

More precise way to think about it wiatqueue lock would be that it's built in a way that also allows it to be used from irq context. It doesn't *force* irq context on all its users.

Signature · 1 line

tejun