From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wu Fengguang Subject: Re: Possible usage of uninitalized task_ratelimit variable in mm/page-writeback.c Date: Mon, 7 Nov 2011 19:15:37 +0800 Message-ID: <20111107111537.GA13453@localhost> References: <20111107081824.GA18221@smp.if.uj.edu.pl> <20111107091704.GA29562@localhost> <20111107110558.GD18221@smp.if.uj.edu.pl> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "linux-mm@kvack.org" , "linux-fsdevel@vger.kernel.org" To: Witold Baryluk Return-path: Received: from mga03.intel.com ([143.182.124.21]:7464 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753830Ab1KGLPj (ORCPT ); Mon, 7 Nov 2011 06:15:39 -0500 Content-Disposition: inline In-Reply-To: <20111107110558.GD18221@smp.if.uj.edu.pl> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Nov 07, 2011 at 07:05:58PM +0800, Witold Baryluk wrote: > On 11-07 17:17, Wu Fengguang wrote: > > On Mon, Nov 07, 2011 at 04:18:24PM +0800, Witold Baryluk wrote: > > > Hi, > > >=20 > > > I found a minor issue when compiling kernel today > > >=20 > > >=20 > > > CC mm/page-writeback.o > > > mm/page-writeback.c: In function =E2=80=98balance_dirty_pages_rat= elimited_nr=E2=80=99: > > > include/trace/events/writeback.h:281:1: warning: =E2=80=98task_ra= telimit=E2=80=99 may be used uninitialized in this function [-Wuninitia= lized] > > > mm/page-writeback.c:1018:16: note: =E2=80=98task_ratelimit=E2=80=99= was declared here > > >=20 > > > Indeed in balance_dirty_pages a task_ratelimit may be not initial= ized > > > (initialization skiped by goto pause;), and then used when callin= g > > > tracing hook. > >=20 > > Witold, thanks for the report! This patch should fix the bug. > >=20 > > Thanks, > > Fengguang > > --- > >=20 > > writeback: fix uninitialized task_ratelimit > >=20 > > In balance_dirty_pages() task_ratelimit may be not initialized > > (initialization skiped by goto pause;), and then used when calling > > tracing hook. > >=20 > > Fix it by moving the task_ratelimit assignment before goto pause. > >=20 > > Reported-by: Witold Baryluk > > Signed-off-by: Wu Fengguang > > --- > > mm/page-writeback.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > >=20 > > --- linux.orig/mm/page-writeback.c 2011-11-07 17:07:04.080000043 +0= 800 > > +++ linux/mm/page-writeback.c 2011-11-07 17:08:43.232000031 +0800 > > @@ -1097,13 +1097,13 @@ static void balance_dirty_pages(struct a > > pos_ratio =3D bdi_position_ratio(bdi, dirty_thresh, > > background_thresh, nr_dirty, > > bdi_thresh, bdi_dirty); > > - if (unlikely(pos_ratio =3D=3D 0)) { > > + task_ratelimit =3D (u64)dirty_ratelimit * > > + pos_ratio >> RATELIMIT_CALC_SHIFT; > > + if (unlikely(task_ratelimit =3D=3D 0)) { > > pause =3D max_pause; > > goto pause; > > } > > - task_ratelimit =3D (u64)dirty_ratelimit * > > - pos_ratio >> RATELIMIT_CALC_SHIFT; > > - pause =3D (HZ * pages_dirtied) / (task_ratelimit | 1); > > + pause =3D (HZ * pages_dirtied) / task_ratelimit; > > if (unlikely(pause <=3D 0)) { > > trace_balance_dirty_pages(bdi, > > dirty_thresh, >=20 > Thanks. >=20 > This is very nice patch, fixes warning, > and simplifies logic. I have no other objections. >=20 > (I do not remember what have bigger priority, * or >> > - i guess * - but additional paranthesis can help. :D ) Good point! Just added/removed parentheses according to your suggestion= s :) Thanks, =46engguang --- writeback: fix uninitialized task_ratelimit In balance_dirty_pages() task_ratelimit may be not initialized (initialization skiped by goto pause;), and then used when calling tracing hook. =46ix it by moving the task_ratelimit assignment before goto pause. Reported-by: Witold Baryluk Signed-off-by: Wu Fengguang --- mm/page-writeback.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- linux.orig/mm/page-writeback.c 2011-11-07 19:11:39.660000042 +0800 +++ linux/mm/page-writeback.c 2011-11-07 19:12:19.856000041 +0800 @@ -1095,13 +1095,13 @@ static void balance_dirty_pages(struct a pos_ratio =3D bdi_position_ratio(bdi, dirty_thresh, background_thresh, nr_dirty, bdi_thresh, bdi_dirty); - if (unlikely(pos_ratio =3D=3D 0)) { + task_ratelimit =3D ((u64)dirty_ratelimit * pos_ratio) >> + RATELIMIT_CALC_SHIFT; + if (unlikely(task_ratelimit =3D=3D 0)) { pause =3D max_pause; goto pause; } - task_ratelimit =3D (u64)dirty_ratelimit * - pos_ratio >> RATELIMIT_CALC_SHIFT; - pause =3D (HZ * pages_dirtied) / (task_ratelimit | 1); + pause =3D HZ * pages_dirtied / task_ratelimit; if (unlikely(pause <=3D 0)) { trace_balance_dirty_pages(bdi, dirty_thresh, -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html