linux-kernel - Re: [PATCH 03/10] writeback: Do not congestion sleep if there are no congested BDIs or significant writeback

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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 8 Sep 2010 23:52:45 +0900
From:	Minchan Kim <minchan.kim@...il.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
	Linux Kernel List <linux-kernel@...r.kernel.org>,
	Rik van Riel <riel@...hat.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Dave Chinner <david@...morbit.com>,
	Chris Mason <chris.mason@...cle.com>,
	Christoph Hellwig <hch@....de>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 03/10] writeback: Do not congestion sleep if there are
 no congested BDIs or significant writeback

On Wed, Sep 08, 2010 at 12:04:03PM +0100, Mel Gorman wrote:
> On Wed, Sep 08, 2010 at 12:25:33AM +0900, Minchan Kim wrote:
> > > + * @zone: A zone to consider the number of being being written back from
> > > + * @sync: SYNC or ASYNC IO
> > > + * @timeout: timeout in jiffies
> > > + *
> > > + * Waits for up to @timeout jiffies for a backing_dev (any backing_dev) to exit
> > > + * write congestion.  If no backing_devs are congested then the number of
> > > + * writeback pages in the zone are checked and compared to the inactive
> > > + * list. If there is no sigificant writeback or congestion, there is no point
> >                                                 and 
> > 
> 
> Why and? "or" makes sense because we avoid sleeping on either condition.

if (nr_bdi_congested[sync]) == 0) {
        if (writeback < inactive / 2) {
                cond_resched();
                ..
                goto out
        }
}

for avoiding sleeping, above two condition should meet. 
So I thought "and" is make sense. 
Am I missing something?

> 
> > > + * in sleeping but cond_resched() is called in case the current process has
> > > + * consumed its CPU quota.
> > > + */
> > > +long wait_iff_congested(struct zone *zone, int sync, long timeout)
> > > +{
> > > +	long ret;
> > > +	unsigned long start = jiffies;
> > > +	DEFINE_WAIT(wait);
> > > +	wait_queue_head_t *wqh = &congestion_wqh[sync];
> > > +
> > > +	/*
> > > +	 * If there is no congestion, check the amount of writeback. If there
> > > +	 * is no significant writeback and no congestion, just cond_resched
> > > +	 */
> > > +	if (atomic_read(&nr_bdi_congested[sync]) == 0) {
> > > +		unsigned long inactive, writeback;
> > > +
> > > +		inactive = zone_page_state(zone, NR_INACTIVE_FILE) +
> > > +				zone_page_state(zone, NR_INACTIVE_ANON);
> > > +		writeback = zone_page_state(zone, NR_WRITEBACK);
> > > +
> > > +		/*
> > > +		 * If less than half the inactive list is being written back,
> > > +		 * reclaim might as well continue
> > > +		 */
> > > +		if (writeback < inactive / 2) {
> > 
> > I am not sure this is best.
> > 
> 
> I'm not saying it is. The objective is to identify a situation where
> sleeping until the next write or congestion clears is pointless. We have
> already identified that we are not congested so the question is "are we
> writing a lot at the moment?". The assumption is that if there is a lot
> of writing going on, we might as well sleep until one completes rather
> than reclaiming more.
> 
> This is the first effort at identifying pointless sleeps. Better ones
> might be identified in the future but that shouldn't stop us making a
> semi-sensible decision now.

nr_bdi_congested is no problem since we have used it for a long time.
But you added new rule about writeback. 

Why I pointed out is that you added new rule and I hope let others know
this change since they have a good idea or any opinions. 
I think it's a one of roles as reviewer.

> 
> > 1. Without considering various speed class storage, could we fix it as half of inactive?
> 
> We don't really have a good means of identifying speed classes of
> storage. Worse, we are considering on a zone-basis here, not a BDI
> basis. The pages being written back in the zone could be backed by
> anything so we cannot make decisions based on BDI speed.

True. So it's why I have below question.
As you said, we don't have enough information in vmscan.
So I am not sure how effective such semi-sensible decision is. 

I think best is to throttle in page-writeback well. 
But I am not a expert about that and don't have any idea. Sorry.
So I can't insist on my nitpick. If others don't have any objection,
I don't mind this, either. 

Wu, Do you have any opinion?

> 
> > 2. Isn't there any writeback throttling on above layer? Do we care of it in here?
> > 
> 
> There are but congestion_wait() and now wait_iff_congested() are part of
> that. We can see from the figures in the leader that congestion_wait()
> is sleeping more than is necessary or smart.
> 
> > Just out of curiosity. 
> > 
> 
> -- 
> Mel Gorman
> Part-time Phd Student                          Linux Technology Center
> University of Limerick                         IBM Dublin Software Lab

-- 
Kind regards,
Minchan Kim
--
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/