linux-kernel - Re: [PATCH] vfs: dcache: cond_resched in shrink_dentry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 Mar 2013 17:39:13 -0700
From:	Greg Thelen <gthelen@...gle.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfs: dcache: cond_resched in shrink_dentry_list

On Mon, Mar 25 2013, Dave Chinner wrote:

> On Mon, Mar 25, 2013 at 10:22:31AM -0700, Greg Thelen wrote:
>> Call cond_resched() from shrink_dentry_list() to preserve
>> shrink_dcache_parent() interactivity.
>> 
>> void shrink_dcache_parent(struct dentry * parent)
>> {
>> 	while ((found = select_parent(parent, &dispose)) != 0)
>> 		shrink_dentry_list(&dispose);
>> }
>> 
>> select_parent() populates the dispose list with dentries which
>> shrink_dentry_list() then deletes.  select_parent() carefully uses
>> need_resched() to avoid doing too much work at once.  But neither
>> shrink_dcache_parent() nor its called functions call cond_resched().
>> So once need_resched() is set select_parent() will return single
>> dentry dispose list which is then deleted by shrink_dentry_list().
>> This is inefficient when there are a lot of dentry to process.  This
>> can cause softlockup and hurts interactivity on non preemptable
>> kernels.
>
> Hi Greg,
>
> I can see how this coul dcause problems, but isn't the problem then
> that shrink_dcache_parent()/select_parent() itself is mishandling
> the need for rescheduling rather than being a problem with
> the shrink_dentry_list() implementation?  i.e. select_parent() is
> aborting batching based on a need for rescheduling, but then not
> doing that itself and assuming that someone else will do the
> reschedule for it?
>
> Perhaps this is a better approach:
>
> -	while ((found = select_parent(parent, &dispose)) != 0)
> +	while ((found = select_parent(parent, &dispose)) != 0) {
>                 shrink_dentry_list(&dispose);
> +		cond_resched();
> +	}
>
> With this, select_parent() stops batching when a resched is needed,
> we dispose of the list as a single batch and only then resched if it
> was needed before we go and grab the next batch. That should fix the
> "small batch" problem without the potential for changing the
> shrink_dentry_list() behaviour adversely for other users....

I considered only modifying shrink_dcache_parent() as you show above.
Either approach fixes the problem I've seen.  My initial approach adds
cond_resched() deeper into shrink_dentry_list() because I thought that
there might a secondary benefit: shrink_dentry_list() would be willing
to give up the processor when working on a huge number of dentry.  This
could improve interactivity during shrinker and umount.  I don't feel
strongly on this and would be willing to test and post the
add-cond_resched-to-shrink_dcache_parent approach.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/