Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 29 Apr 2020 15:35:32 +0200
From:   Michal Hocko <mhocko@kernel.org>
To:     Vaneet Narang <v.narang@samsung.com>
Cc:     Maninder Singh <maninder1.s@samsung.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        AMIT SAHRAWAT <a.sahrawat@samsung.com>
Subject: Re: (2) [PATCH 1/1] mm/vmscan.c: change prototype for
 shrink_page_list
Message-ID: <20200429133532.GF28637@dhcp22.suse.cz>
References: <20200429130912.GE28637@dhcp22.suse.cz>
 <20200429120951.GC28637@dhcp22.suse.cz>
 <1588139967-19012-1-git-send-email-maninder1.s@samsung.com>
 <20200429125323epcms5p67a539511c573cd598d78b726503e3204@epcms5p6>
 <CGME20200429055946epcas5p2d5faf2b320913d59a4a8380cb017053c@epcms5p8>
 <20200429132940epcms5p81e75e09469b62253ff512222c568556f@epcms5p8>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200429132940epcms5p81e75e09469b62253ff512222c568556f@epcms5p8>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed 29-04-20 18:59:40, Vaneet Narang wrote:
> Hi Michal,?
> 
> >>?>
> >>?>Acked-by:?Michal?Hocko?<mhocko@suse.com>
> >>?>
> >>?>Is?there?any?reason?to?move?declarations?here?
> >>?>
> >>?
> >>?"unsigned?int?ret"?was?changed?mistakenely,?sending?V2.
> >>?and?"unsigned?int?nr_reclaimed"?is?changed?to?remove?hole.
> ?
> >Could?you?be?more?specific??Have?you?seen?a?better?stack?allocation
> >footprint?
> 
> We didn't check stack allocation footprint, we did changes just by looking into the code.
> we thought changing reclaimed type from long to int on 64 bit platform will add 
> hole of 4 bytes between two stack variables nr_reclaimed & nr_taken.
> 
> So we tried to remove that hole by packing it with bool.
> 
>  	unsigned long nr_scanned;                 --> Size and alignment 8 byte for long 
> -	unsigned long nr_reclaimed = 0;           --> Changing to int will make its size as 4  
>  	unsigned long nr_taken;                   --> nr_taken needs alignment of 8 so will add hole.
>  	struct reclaim_stat stat;
>  	int file = is_file_lru(lru);
>  	enum vm_event_item item;
>  	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> +	unsigned int nr_reclaimed = 0;            --> So moving to this place to pack it along with bool 
>  	bool stalled = false;
> 
> 
> Overall stack footprint might not change as compiler makes function stack pointer as 16 byte aligned but we did this 
> as we normally follow this coding convention when defining structures or stack variables. 

My understanding is that gcc can and does tricks when allocating space
for local variables. It can use registers and there is no dictated
structure on the placing of variable or ordering (unlike for
structures).

Anyway, I would prefer if the patch was doing one thing at the time.
If you can see some (have a look ./scripts/bloat-o-meter) improvements
from reordering, make it a separate patch with some numbers attached.
-- 
Michal Hocko
SUSE Labs