linux-kernel - Re: [patch 03/21] use an array for the LRU pagevecs

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]

Message-Id: <20080301153941.528A.KOSAKI.MOTOHIRO@jp.fujitsu.com>
Date:	Sat, 01 Mar 2008 16:02:04 +0900
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	Andy Whitcroft <apw@...dowen.org>
Cc:	kosaki.motohiro@...fujitsu.com, Rik van Riel <riel@...hat.com>,
	linux-kernel@...r.kernel.org,
	Lee Schermerhorn <Lee.Schermerhorn@...com>, linux-mm@...ck.org
Subject: Re: [patch 03/21] use an array for the LRU pagevecs

Hi Andy

sorry, almost mistake maked by me. 

> >  #define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
> >  
> > +static inline int is_active_lru(enum lru_list l)
> > +{
> > +	if (l == LRU_ACTIVE)
> > +		return 1;
> > +	return 0;
> 
> Can this not be:
> 
> 	return (l == LRU_ACTIVE);

yes, your code is more better.

Thanks.


> > @@ -98,6 +97,19 @@ void put_pages_list(struct list_head *pa
> >  EXPORT_SYMBOL(put_pages_list);
> >  
> >  /*
> > + * Returns the LRU list a page should be on.
> > + */
> > +enum lru_list page_lru(struct page *page)
> > +{
> > +	enum lru_list lru = LRU_BASE;
> > +
> > +	if (PageActive(page))
> > +		lru += LRU_ACTIVE;
> 
> This is introducing an assumption that LRU_BASE and LRU_INACTIVE are
> synonymous?  Would it not be better to explicitly use LRU_INACTIVE:

Yes.
this patch series assume LRU_BASE == LRU_INACTIVE.

> So either:
> 
> 	if (PageActive(page))
> 		lru = LRU_ACTIVE;
> 	else
> 		lru = LRU_INACTIVE;

if add LRU_FILE, this statement makes messy.
key point of indexed arraynize is able to arithmetic calculationed.


> Or if (as I assume) this is later going to have other mappings added in
> you could do it more like the following.  This should produce identicle
> asm, but removes any possiblity of LRU_BASE/INACTIVE slippage breaking
> things:
> 
> 	enum lru_list lru = LRU_INACTIVE;
> 
> 	if (PageActive(page))
> 		lru += (LRU_ACTIVE - LRU_INACTIVE);

I think your code is more descriptive, but not best simply.
I think LRU_BASE == LRU_INACTIVE assumption make simply code.

> > -	struct pagevec *pvec = &get_cpu_var(lru_add_active_pvecs);
> > +	if (PageActive(page)) {
> > +		ClearPageActive(page);
> > +	}
> 
> {}'s are not needed here.

agghh
thank you good catch ;)


Thank you again for your very careful reviews.


- kosaki


--
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/