| Message ID | 5342F083.5020509@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
>> This change hwpoison and migration tag number. maybe ok, maybe not. > > Though depending on config can't these tag numbers change anyway? I don't think distro disable any of these. >> I'd suggest to use younger number than hwpoison. >> (That's why hwpoison uses younger number than migration) > > So I can, but the way these are defined makes the results seem pretty > terrible: > > #define SWP_MIGRATION_WRITE (MAX_SWAPFILES + SWP_HWPOISON_NUM \ > + SWP_MVOLATILE_PURGED_NUM + 1) > > Particularly when: > #define MAX_SWAPFILES ((1 << MAX_SWAPFILES_SHIFT) \ > - SWP_MIGRATION_NUM \ > - SWP_HWPOISON_NUM \ > - SWP_MVOLATILE_PURGED_NUM \ > ) > > Its a lot of unnecessary mental gymnastics. Yuck. > > Would a general cleanup like the following be ok to try to make this > more extensible? > > thanks > -john > > diff --git a/include/linux/swap.h b/include/linux/swap.h > index 3507115..21387df 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -49,29 +49,38 @@ static inline int current_is_kswapd(void) > * actions on faults. > */ > > +enum { > + /* > + * NOTE: We use the high bits here (subtracting from > + * 1<<MAX_SWPFILES_SHIFT), so to preserve the values insert > + * new entries here at the top of the enum, not at the bottom > + */ > +#ifdef CONFIG_MEMORY_FAILURE > + SWP_HWPOISON_NR, > +#endif > +#ifdef CONFIG_MIGRATION > + SWP_MIGRATION_READ_NR, > + SWP_MIGRATION_WRITE_NR, > +#endif > + SWP_MAX_NR, > +}; > +#define MAX_SWAPFILES ((1 << MAX_SWAPFILES_SHIFT) - SWP_MAX_NR) > + I don't see any benefit of this code. At least, SWP_MAX_NR is suck. The name doesn't match the actual meanings. -- 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/
On 04/07/2014 03:14 PM, KOSAKI Motohiro wrote: >>> This change hwpoison and migration tag number. maybe ok, maybe not. >> Though depending on config can't these tag numbers change anyway? > I don't think distro disable any of these. Well, it still shouldn't break if the config options are turned off. This isn't some subtle userspace visible ABI, is it? I'm fine with keeping the values the same, but it just seems worrying if this logic is so fragile. >>> I'd suggest to use younger number than hwpoison. >>> (That's why hwpoison uses younger number than migration) >> So I can, but the way these are defined makes the results seem pretty >> terrible: >> >> #define SWP_MIGRATION_WRITE (MAX_SWAPFILES + SWP_HWPOISON_NUM \ >> + SWP_MVOLATILE_PURGED_NUM + 1) >> >> Particularly when: >> #define MAX_SWAPFILES ((1 << MAX_SWAPFILES_SHIFT) \ >> - SWP_MIGRATION_NUM \ >> - SWP_HWPOISON_NUM \ >> - SWP_MVOLATILE_PURGED_NUM \ >> ) >> >> Its a lot of unnecessary mental gymnastics. Yuck. >> >> Would a general cleanup like the following be ok to try to make this >> more extensible? >> >> thanks >> -john >> >> diff --git a/include/linux/swap.h b/include/linux/swap.h >> index 3507115..21387df 100644 >> --- a/include/linux/swap.h >> +++ b/include/linux/swap.h >> @@ -49,29 +49,38 @@ static inline int current_is_kswapd(void) >> * actions on faults. >> */ >> >> +enum { >> + /* >> + * NOTE: We use the high bits here (subtracting from >> + * 1<<MAX_SWPFILES_SHIFT), so to preserve the values insert >> + * new entries here at the top of the enum, not at the bottom >> + */ >> +#ifdef CONFIG_MEMORY_FAILURE >> + SWP_HWPOISON_NR, >> +#endif >> +#ifdef CONFIG_MIGRATION >> + SWP_MIGRATION_READ_NR, >> + SWP_MIGRATION_WRITE_NR, >> +#endif >> + SWP_MAX_NR, >> +}; >> +#define MAX_SWAPFILES ((1 << MAX_SWAPFILES_SHIFT) - SWP_MAX_NR) >> + > I don't see any benefit of this code. At least, SWP_MAX_NR is suck. So it makes adding new special swap types (like SWP_MVOLATILE_PURGED) much cleaner. If we need to preserve the actual values for SWP_HWPOSIN and SWP_MIGRATION_* as you suggested earlier, the cleanup above makes doing so when adding a new type much easier. For example adding the MVOLATILE_PURGED value (without effecting the values of HWPOSIN or MIGRATION_*) is only: @@ -55,6 +55,7 @@ enum { * 1<<MAX_SWPFILES_SHIFT), so to preserve the values insert * new entries here at the top of the enum, not at the bottom */ + SWP_MVOLATILE_PURGED_NR, #ifdef CONFIG_MEMORY_FAILURE SWP_HWPOISON_NR, #endif @@ -81,6 +82,10 @@ enum { #define SWP_HWPOISON (MAX_SWAPFILES + SWP_HWPOISON_NR) #endif +/* + * Purged volatile range pages + */ +#define SWP_MVOLATILE_PURGED (MAX_SWAPFILES + SWP_MVOLATILE_PURGED_NR) That's *much* nicer when compared with modifying every value to subtract the extra entry, as it was done before. > The name doesn't match the actual meanings. Would SWP_MAX_SPECIAL_TYPE_NR be a better name? Do you have other suggestions? thanks -john -- 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/
diff --git a/include/linux/swap.h b/include/linux/swap.h index 3507115..21387df 100644 --- a/include/linux/swap.h +++ b/include/linux/swap.h @@ -49,29 +49,38 @@ static inline int current_is_kswapd(void) * actions on faults. */ +enum { + /* + * NOTE: We use the high bits here (subtracting from + * 1<<MAX_SWPFILES_SHIFT), so to preserve the values insert + * new entries here at the top of the enum, not at the bottom + */ +#ifdef CONFIG_MEMORY_FAILURE + SWP_HWPOISON_NR, +#endif +#ifdef CONFIG_MIGRATION + SWP_MIGRATION_READ_NR, + SWP_MIGRATION_WRITE_NR, +#endif + SWP_MAX_NR, +}; +#define MAX_SWAPFILES ((1 << MAX_SWAPFILES_SHIFT) - SWP_MAX_NR) + /* * NUMA node memory migration support */ #ifdef CONFIG_MIGRATION -#define SWP_MIGRATION_NUM 2 -#define SWP_MIGRATION_READ (MAX_SWAPFILES + SWP_HWPOISON_NUM) -#define SWP_MIGRATION_WRITE (MAX_SWAPFILES + SWP_HWPOISON_NUM + 1) -#else -#define SWP_MIGRATION_NUM 0 +#define SWP_MIGRATION_READ (MAX_SWAPFILES + SWP_MIGRATION_READ_NR) +#define SWP_MIGRATION_WRITE (MAX_SWAPFILES + SWP_MIGRATION_WRITE_NR) #endif /* * Handling of hardware poisoned pages with memory corruption. */ #ifdef CONFIG_MEMORY_FAILURE -#define SWP_HWPOISON_NUM 1 -#define SWP_HWPOISON MAX_SWAPFILES -#else -#define SWP_HWPOISON_NUM 0 +#define SWP_HWPOISON (MAX_SWAPFILES + SWP_HWPOISON_NR) #endif -#define MAX_SWAPFILES \ - ((1 << MAX_SWAPFILES_SHIFT) - SWP_MIGRATION_NUM - SWP_HWPOISON_NUM) /* * Magic header for a swap area. The first part of the union is