Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751504Ab0FVXHi (ORCPT ); Tue, 22 Jun 2010 19:07:38 -0400 Received: from mail-iw0-f174.google.com ([209.85.214.174]:44706 "EHLO mail-iw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750829Ab0FVXHh convert rfc822-to-8bit (ORCPT ); Tue, 22 Jun 2010 19:07:37 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=LAB20SgThMesAnYkU2YufXmZNHOndkVDP4zuFSO5/RvHeEEiOMCfS9XVbcbOw92u3e +5l/Z0xJONvGORYMhzSBSsCwWpgchQIzel8zBaynjKdc6iwnBCU/o28nrNMOek/wO6Cw XkuAVkHZi2hT6YIjLgnyVqRoO5lgErplh/7rs= MIME-Version: 1.0 In-Reply-To: <20100622213301.GA26285@cmpxchg.org> References: <20100622112416.B554.A69D9226@jp.fujitsu.com> <20100622114739.B563.A69D9226@jp.fujitsu.com> <20100622213301.GA26285@cmpxchg.org> Date: Wed, 23 Jun 2010 08:07:34 +0900 Message-ID: Subject: Re: [Patch] Call cond_resched() at bottom of main look in balance_pgdat() From: Minchan Kim To: Johannes Weiner Cc: KOSAKI Motohiro , Larry Woodman , linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4116 Lines: 100 On Wed, Jun 23, 2010 at 6:33 AM, Johannes Weiner wrote: > On Tue, Jun 22, 2010 at 01:29:17PM +0900, Minchan Kim wrote: >> On Tue, Jun 22, 2010 at 12:23 PM, KOSAKI Motohiro >> wrote: >> >> >> Kosaki's patch's goal is that kswap doesn't yield cpu if the zone doesn't meet its >> >> >> min watermark to avoid failing atomic allocation. >> >> >> But this patch could yield kswapd's time slice at any time. >> >> >> Doesn't the patch break your goal in bb3ab59683? >> >> > >> >> > No. it don't break. >> >> > >> >> > Typically, kswapd periodically call shrink_page_list() and it call >> >> > cond_resched() even if bb3ab59683 case. >> >> >> >> Hmm. If it is, bb3ab59683 is effective really? >> >> >> >> The bb3ab59683's goal is prevent CPU yield in case of free < min_watermark. >> >> But shrink_page_list can yield cpu from kswapd at any time. >> >> So I am not sure what is bb3ab59683's benefit. >> >> Did you have any number about bb3ab59683's effectiveness? >> >> (Of course, I know it's very hard. Just out of curiosity) >> >> >> >> As a matter of fact, when I saw this Larry's patch, I thought it would >> >> be better to revert bb3ab59683. Then congestion_wait could yield CPU >> >> to other process. >> >> >> >> What do you think about? >> > >> > No. The goal is not prevent CPU yield. The goal is avoid unnecessary >> > _long_ sleep (i.e. congestion_wait(BLK_RW_ASYNC, HZ/10)). >> >> I meant it. >> >> > Anyway we can't refuse CPU yield on UP. it lead to hangup ;) >> > >> > What do you mean the number? If it mean how much reduce congestion_wait(), >> > it was posted a lot of time. If it mean how much reduce page allocation >> > failure bug report, I think it has been observable reduced since half >> > years ago. >> >> I meant second. >> Hmm. I doubt it's observable since at that time, Mel had posted many >> patches to reduce page allocation fail. bb3ab59683 was just one of >> them. >> >> > >> > If you have specific worried concern, can you please share it? >> > >> >> My concern is that I don't want to add new band-aid on uncertain >> feature to solve >> regression of uncertain feature.(Sorry for calling Larry's patch as band-aid.). >> If we revert bb3ab59683, congestion_wait in balance_pgdat could yield >> cpu from kswapd. >> >> If you insist on bb3ab59683's effective and have proved it at past, I >> am not against it. >> >> And If it's regression of bb3ab59683, Doesn't it make sense following as? >> It could restore old behavior. >> >> --- >>                  * OK, kswapd is getting into trouble.  Take a nap, then take >>                  * another pass across the zones. >>                  */ >>                 if (total_scanned && (priority < DEF_PRIORITY - 2)) { >>                         if (has_under_min_watermark_zone) { >>                                 count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT); >>                                 /* allowing CPU yield to go on >> watchdog or OOMed task */ >>                                 cond_resched(); > > We have two things here: one is waiting for some IO to complete, which > we skip if we are in a hurry.  The other thing is that we have a > potentially long-running loop with no garuanteed rescheduling point in > it.  I would rather not mix up those two and let this cond_resched() > for #2 stand on it's own and be self-explanatory. > > So, > > Acked-by: Johannes Weiner > > to Larry's patch (or KOSAKI-san's version of it for that matter). > Okay. As I hear Kosaki and Hannes opinions, I was paranoid. Thanks for good comment!, Kosaki and Hannes. Feel free to add my sign to Kosaki's version(I like detailed description :) ) Reviewed-by: Minchan Kim -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/