| Message ID | CA+55aFy54iDN56FDJAz3A8epRvKECVO+nL5LaMxdmKrFEOm05w@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 10:52 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Jun 23, 2016 at 10:44 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> The thread_info->tsk pointer, that was one of the most critical issues >> and the main raison d'être of the thread_info, has been replaced on >> x86 by just using the per-cpu "current_task". Yes,.there are probably >> more than a few "ti->task" users left for legacy reasons, harking back >> to when the thread-info was cheaper to access, but it shouldn't be a >> big deal. > > Ugh. Looking around at this, it turns out that a great example of this > kind of legacy issue is the debug_mutex stuff. > > It uses "struct thread_info *" as the owner pointer, and there is _no_ > existing reason for it. In fact, in every single place it actually > wants the task_struct, and it does task_thread_info(task) just to > convert it to the thread-info, and then converts it back with > "ti->task". Heh, yeah, that looks like a nice clean-up. > So the attached patch seems to be the right thing to do regardless of > this whole discussion. Why does __mutex_lock_common() have "task" as a stack variable? It's only assigned at the start, and is always "current". (I only noticed from the patch changing "current_thread_info()" and "task_thread_info(task)" both to "task".) -Kees
On 06/23, Linus Torvalds wrote: > > Ugh. Looking around at this, it turns out that a great example of this > kind of legacy issue is the debug_mutex stuff. Heh ;) I am looking at it too. > It uses "struct thread_info *" as the owner pointer, and there is _no_ > existing reason for it. In fact, in every single place it actually > wants the task_struct, and it does task_thread_info(task) just to > convert it to the thread-info, and then converts it back with > "ti->task". Even worse, this task is always "current" afaics, so > So the attached patch seems to be the right thing to do regardless of > this whole discussion. I think we should simply remove this argument. And probably kill task_struct->blocked_on? I do not see the point of this task->blocked_on != waiter check. Oleg.
On Thu, Jun 23, 2016 at 10:52:58AM -0700, Linus Torvalds wrote: > Ugh. Looking around at this, it turns out that a great example of this > kind of legacy issue is the debug_mutex stuff. > > It uses "struct thread_info *" as the owner pointer, and there is _no_ > existing reason for it. In fact, in every single place it actually > wants the task_struct, and it does task_thread_info(task) just to > convert it to the thread-info, and then converts it back with > "ti->task". > > So the attached patch seems to be the right thing to do regardless of > this whole discussion. Yeah, that looks fine. Want me to take it or will you just commit?
On Thu, Jun 23, 2016 at 11:00:08AM -0700, Kees Cook wrote: > > Why does __mutex_lock_common() have "task" as a stack variable? That's actually a fairly common thing to do. The reason is that 'current' is far more expensive to evaluate than a local variable.
On Thu, Jun 23, 2016 at 08:12:16PM +0200, Oleg Nesterov wrote: > > And probably kill task_struct->blocked_on? I do not see the point of > this task->blocked_on != waiter check. I think that came about because of PI and or deadlock detection. Of course, the current mutex code doesn't have anything like that these days, and rt_mutex has task_struct::pi_blocked_on.
On Thu, Jun 23, 2016 at 11:53 AM, Peter Zijlstra <peterz@infradead.org> wrote: > On Thu, Jun 23, 2016 at 10:52:58AM -0700, Linus Torvalds wrote: >> Ugh. Looking around at this, it turns out that a great example of this >> kind of legacy issue is the debug_mutex stuff. >> >> It uses "struct thread_info *" as the owner pointer, and there is _no_ >> existing reason for it. In fact, in every single place it actually >> wants the task_struct, and it does task_thread_info(task) just to >> convert it to the thread-info, and then converts it back with >> "ti->task". >> >> So the attached patch seems to be the right thing to do regardless of >> this whole discussion. > > Yeah, that looks fine. Want me to take it or will you just commit? PeterZ, mind if I split it into a couple of patches, test it, and add it to my series? --Andy
On Thu, Jun 23, 2016 at 12:09:53PM -0700, Andy Lutomirski wrote: > On Thu, Jun 23, 2016 at 11:53 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > Yeah, that looks fine. Want me to take it or will you just commit? > > PeterZ, mind if I split it into a couple of patches, test it, and add > it to my series? Not at all, keep me on Cc?
On Thu, Jun 23, 2016 at 11:53 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> >> So the attached patch seems to be the right thing to do regardless of >> this whole discussion. > > Yeah, that looks fine. Want me to take it or will you just commit? I'm committing these trivial non-semantic patches, I'm actually running the kernel without any ti->task pointer now (the previous patch I sent out). So I'll do the mutex debug patch and the stack dump patch as just he obvious cleanup patches. Those are the "purely legacy reasons for a bad calling convention", and I'm ok with those during the rc series to make it easier for people to play around with this. With he goal being that I'm hoping that we can then actually get rid of this (at least on x86-64, even if we leave it in some other architectures) in 4.8. Linus
diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c index 3ef3736002d8..9c951fade415 100644 --- a/kernel/locking/mutex-debug.c +++ b/kernel/locking/mutex-debug.c @@ -49,21 +49,21 @@ void debug_mutex_free_waiter(struct mutex_waiter *waiter) } void debug_mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter, - struct thread_info *ti) + struct task_struct *task) { SMP_DEBUG_LOCKS_WARN_ON(!spin_is_locked(&lock->wait_lock)); /* Mark the current thread as blocked on the lock: */ - ti->task->blocked_on = waiter; + task->blocked_on = waiter; } void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter, - struct thread_info *ti) + struct task_struct *task) { DEBUG_LOCKS_WARN_ON(list_empty(&waiter->list)); - DEBUG_LOCKS_WARN_ON(waiter->task != ti->task); - DEBUG_LOCKS_WARN_ON(ti->task->blocked_on != waiter); - ti->task->blocked_on = NULL; + DEBUG_LOCKS_WARN_ON(waiter->task != task); + DEBUG_LOCKS_WARN_ON(task->blocked_on != waiter); + task->blocked_on = NULL; list_del_init(&waiter->list); waiter->task = NULL; diff --git a/kernel/locking/mutex-debug.h b/kernel/locking/mutex-debug.h index 0799fd3e4cfa..d06ae3bb46c5 100644 --- a/kernel/locking/mutex-debug.h +++ b/kernel/locking/mutex-debug.h @@ -20,9 +20,9 @@ extern void debug_mutex_wake_waiter(struct mutex *lock, extern void debug_mutex_free_waiter(struct mutex_waiter *waiter); extern void debug_mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter, - struct thread_info *ti); + struct task_struct *task); extern void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter, - struct thread_info *ti); + struct task_struct *task); extern void debug_mutex_unlock(struct mutex *lock); extern void debug_mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key); diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index 79d2d765a75f..a70b90db3909 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -537,7 +537,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, goto skip_wait; debug_mutex_lock_common(lock, &waiter); - debug_mutex_add_waiter(lock, &waiter, task_thread_info(task)); + debug_mutex_add_waiter(lock, &waiter, task); /* add waiting tasks to the end of the waitqueue (FIFO): */ list_add_tail(&waiter.list, &lock->wait_list); @@ -584,7 +584,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, } __set_task_state(task, TASK_RUNNING); - mutex_remove_waiter(lock, &waiter, current_thread_info()); + mutex_remove_waiter(lock, &waiter, task); /* set it to 0 if there are no waiters left: */ if (likely(list_empty(&lock->wait_list))) atomic_set(&lock->count, 0); @@ -605,7 +605,7 @@ skip_wait: return 0; err: - mutex_remove_waiter(lock, &waiter, task_thread_info(task)); + mutex_remove_waiter(lock, &waiter, task); spin_unlock_mutex(&lock->wait_lock, flags); debug_mutex_free_waiter(&waiter); mutex_release(&lock->dep_map, 1, ip); diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h index 5cda397607f2..a68bae5e852a 100644 --- a/kernel/locking/mutex.h +++ b/kernel/locking/mutex.h @@ -13,7 +13,7 @@ do { spin_lock(lock); (void)(flags); } while (0) #define spin_unlock_mutex(lock, flags) \ do { spin_unlock(lock); (void)(flags); } while (0) -#define mutex_remove_waiter(lock, waiter, ti) \ +#define mutex_remove_waiter(lock, waiter, task) \ __list_del((waiter)->list.prev, (waiter)->list.next) #ifdef CONFIG_MUTEX_SPIN_ON_OWNER