From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751416AbbIGNwL (ORCPT ); Mon, 7 Sep 2015 09:52:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58921 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751050AbbIGNwJ (ORCPT ); Mon, 7 Sep 2015 09:52:09 -0400 Date: Mon, 7 Sep 2015 15:49:24 +0200 From: Oleg Nesterov To: Al Viro , Linus Torvalds Cc: Eric Dumazet , "linux-kernel@vger.kernel.org" , Andrew Morton , Thomas Gleixner , Ingo Molnar , Maciej =?utf-8?Q?=C5=BBenczykowski?= Subject: [PATCH? v2] fput: don't abuse task_work_add() too much Message-ID: <20150907134924.GA24254@redhat.com> References: <1440816150.8932.123.camel@edumazet-glaptop2.roam.corp.google.com> <20150829124921.GA14973@redhat.com> <20150905053536.GD22011@ZenIV.linux.org.uk> <20150907122709.GA31811@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150907122709.GA31811@redhat.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/07, Oleg Nesterov wrote: > > Oh, I disagree. But I guess I can't convince you/Eric/Linus, so I have > to shut up. > > > Damn. But I can't relax ;) Al, Linus, could you comment the patch below? > > Not for inclusion, lacks the changelog/testing, fput() can be simplified. > But as you can see it is simple. With this patch task_work_add(____fput) > will be called only once by (say) do_exit() path. ->fput_list does not > need any serialization / atomic ops / etc. Probably we also need to move > cond_resched() from task_work_run() to ____fput() after this patch. > > Again, it is not that I think this actually makes sense, but since you > dislike these 275ms... > > What do you think? Yes, task_struct->fput_list is ugly. We can avoid it, but then we need another ->next pointer in struct file. Perhaps we can reuse ->f_version? This way the change looks really simple and not too bad to me. Although I am not sure you will agree. Oleg. --- diff --git a/fs/file_table.c b/fs/file_table.c index 294174d..c34b666 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -241,7 +241,15 @@ static void delayed_fput(struct work_struct *unused) static void ____fput(struct callback_head *work) { - __fput(container_of(work, struct file, f_u.fu_rcuhead)); + struct file *file = container_of(work, struct file, f_u.fu_rcuhead); + struct file *next; + + do { + next = file->f_next_put; + __fput(file); + file = next; + + } while (file); } /* @@ -267,9 +275,21 @@ void fput(struct file *file) struct task_struct *task = current; if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { + struct callback_head *work = READ_ONCE(task->task_works); + struct file *prev; + + if (work && work->func == ____fput) { + prev = container_of(work, struct file, f_u.fu_rcuhead); + file->f_next_put = prev->f_next_put; + prev->f_next_put = file; + return; + } + init_task_work(&file->f_u.fu_rcuhead, ____fput); - if (!task_work_add(task, &file->f_u.fu_rcuhead, true)) + if (!task_work_add(task, &file->f_u.fu_rcuhead, true)) { + file->f_next_put = NULL; return; + } /* * After this task has run exit_task_work(), * task_work_add() will fail. Fall through to delayed diff --git a/include/linux/fs.h b/include/linux/fs.h index 0774487..9381527 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -849,7 +849,10 @@ struct file { const struct cred *f_cred; struct file_ra_state f_ra; - u64 f_version; + union { + u64 f_version; + struct file *f_next_put; + }; #ifdef CONFIG_SECURITY void *f_security; #endif