Messages in this thread | | | Date | Sat, 30 May 2009 20:16:51 +0900 (JST) | Subject | Re: [PATCH 2/4] modify swap_map and add SWAP_HAS_CACHE flag. | From | "KAMEZAWA Hiroyuki" <> |
| |
Balbir Singh wrote: >> #define SWAP_CLUSTER_MAX 32 >> >> -#define SWAP_MAP_MAX 0x7fff >> -#define SWAP_MAP_BAD 0x8000 >> - >> +#define SWAP_MAP_MAX 0x7ffe >> +#define SWAP_MAP_BAD 0x7fff >> +#define SWAP_HAS_CACHE 0x8000 /* There is a swap cache of entry. */ > > Why count, can't we use swp->flags? >
Hmm ? swap_map just only a "unsiged short" value per entry..sorry, I can't catch what you mention to.
>> +#define SWAP_COUNT_MASK (~SWAP_HAS_CACHE) >> /* >> * The in-memory structure used to track swap areas. >> */ >> @@ -300,7 +301,7 @@ extern long total_swap_pages; >> extern void si_swapinfo(struct sysinfo *); >> extern swp_entry_t get_swap_page(void); >> extern swp_entry_t get_swap_page_of_type(int); >> -extern int swap_duplicate(swp_entry_t); >> +extern void swap_duplicate(swp_entry_t); >> extern int swapcache_prepare(swp_entry_t); >> extern int valid_swaphandles(swp_entry_t, unsigned long *); >> extern void swap_free(swp_entry_t); >> @@ -372,9 +373,12 @@ static inline void show_swap_cache_info( >> } >> >> #define free_swap_and_cache(swp) is_migration_entry(swp) >> -#define swap_duplicate(swp) is_migration_entry(swp) >> #define swapcache_prepare(swp) is_migration_entry(swp) >> >> +static inline void swap_duplicate(swp_entry_t swp) >> +{ >> +} >> + >> static inline void swap_free(swp_entry_t swp) >> { >> } >> Index: new-trial-swapcount2/mm/swapfile.c >> =================================================================== >> --- new-trial-swapcount2.orig/mm/swapfile.c >> +++ new-trial-swapcount2/mm/swapfile.c >> @@ -53,6 +53,26 @@ static struct swap_info_struct swap_info >> >> static DEFINE_MUTEX(swapon_mutex); >> >> +/* For reference count accounting in swap_map */ >> +static inline int swap_count(unsigned short ent) >> +{ >> + return ent & SWAP_COUNT_MASK; >> +} >> + >> +static inline int swap_has_cache(unsigned short ent) >> +{ >> + return ent & SWAP_HAS_CACHE; >> +} >> + >> +static inline unsigned short make_swap_count(int count, int has_cache) >> +{ >> + unsigned short ret = count; >> + >> + if (has_cache) >> + return SWAP_HAS_CACHE | ret; >> + return ret; >> +} > > make_swap_count() does not make too much sense in terms of the name > for the function. Should it be called generate_swap_count or > assign_swap_count_info?
Hmm ? ok, rename this as generate_swap_count(). or generate_swapmap_info().
>> + >> /* >> * We need this because the bdev->unplug_fn can sleep and we cannot >> * hold swap_lock while calling the unplug_fn. And swap_lock >> @@ -167,7 +187,8 @@ static int wait_for_discard(void *word) >> #define SWAPFILE_CLUSTER 256 >> #define LATENCY_LIMIT 256 >> >> -static inline unsigned long scan_swap_map(struct swap_info_struct *si) >> +static inline unsigned long scan_swap_map(struct swap_info_struct *si, >> + int cache) > > Can we please use bool for readability or even better an enum? > ok, enum.
> Looks good at first glance otherwise. I think distinguishing between > the counts is good, but also complex. Overall the patch is useful. > Thank you for review.
-Kame
> -- > Balbir >
| |