| Message ID | 20240716033516.606582-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/hwpoison: reset hwpoison filter parameters in pfn_inject_exit() | expand |
On Tue 16-07-24 11:35:16, Miaohe Lin wrote: > When hwpoison_inject module is removed, hwpoison_filter_* parameters > should be reset. Otherwise these parameters will have non-default values > at next insmod time. There is a clear layering broken here. We have mm/memory-failure.c using values and mm/hwpoison-inject.c defining the values. Both with a potentially different life time. Shouldn't that be fix instead?
On 2024/7/16 16:38, Michal Hocko wrote: > On Tue 16-07-24 11:35:16, Miaohe Lin wrote: >> When hwpoison_inject module is removed, hwpoison_filter_* parameters >> should be reset. Otherwise these parameters will have non-default values >> at next insmod time. > > There is a clear layering broken here. We have mm/memory-failure.c using > values and mm/hwpoison-inject.c defining the values. Both with a > potentially different life time. Shouldn't that be fix instead? In fact, we have mm/memory-failure.c defining and using these values while they can only be modified through mm/hwpoison-inject.c from userspace. The common usecase should be: 1. User set hwpoison filter parameters first through mm/hwpoison-inject.c. 2. Then doing memory hwpoison test through mm/hwpoison-inject.c. hwpoison_filter_* parameters are only used for test from userspace. From this perspective, this potentially different life time doesn't break anything. Or am I miss something? Thanks. .
On Wed 17-07-24 10:23:06, Miaohe Lin wrote: > On 2024/7/16 16:38, Michal Hocko wrote: > > On Tue 16-07-24 11:35:16, Miaohe Lin wrote: > >> When hwpoison_inject module is removed, hwpoison_filter_* parameters > >> should be reset. Otherwise these parameters will have non-default values > >> at next insmod time. > > > > There is a clear layering broken here. We have mm/memory-failure.c using > > values and mm/hwpoison-inject.c defining the values. Both with a > > potentially different life time. Shouldn't that be fix instead? > > In fact, we have mm/memory-failure.c defining and using these values while they can > only be modified through mm/hwpoison-inject.c from userspace. Yes, this is exactly what I mean by broken layering that should be fixed. > The common usecase should be: > > 1. User set hwpoison filter parameters first through mm/hwpoison-inject.c. > 2. Then doing memory hwpoison test through mm/hwpoison-inject.c. Why does this need to be done through different modules? Why it cannot be part of the memory-filure.c?
On 2024/7/17 14:18, Michal Hocko wrote: > On Wed 17-07-24 10:23:06, Miaohe Lin wrote: >> On 2024/7/16 16:38, Michal Hocko wrote: >>> On Tue 16-07-24 11:35:16, Miaohe Lin wrote: >>>> When hwpoison_inject module is removed, hwpoison_filter_* parameters >>>> should be reset. Otherwise these parameters will have non-default values >>>> at next insmod time. >>> >>> There is a clear layering broken here. We have mm/memory-failure.c using >>> values and mm/hwpoison-inject.c defining the values. Both with a >>> potentially different life time. Shouldn't that be fix instead? >> >> In fact, we have mm/memory-failure.c defining and using these values while they can >> only be modified through mm/hwpoison-inject.c from userspace. > > Yes, this is exactly what I mean by broken layering that should be > fixed. > >> The common usecase should be: >> >> 1. User set hwpoison filter parameters first through mm/hwpoison-inject.c. >> 2. Then doing memory hwpoison test through mm/hwpoison-inject.c. > > Why does this need to be done through different modules? Why it cannot > be part of the memory-filure.c? This is a bold idea for me. :) I think it can be part of the memory-filure.c. So CONFIG_HWPOISON_INJECT should be removed from the world and then make hwpoison-inject default on when MEMORY_FAILURE is configured? Thanks. .
On Wed 17-07-24 14:59:40, Miaohe Lin wrote: > On 2024/7/17 14:18, Michal Hocko wrote: [...] > > Why does this need to be done through different modules? Why it cannot > > be part of the memory-filure.c? > > This is a bold idea for me. :) I think it can be part of the memory-filure.c. > So CONFIG_HWPOISON_INJECT should be removed from the world and then make > hwpoison-inject default on when MEMORY_FAILURE is configured? I would start by moving code into mm/hwpoison-inject.c. Whether this should also remove CONFIG_HWPOISON_INJECT is a different question. I am not really sure a debugfs interface warrants a config option. Anyway two things for two separate patches
On 2024/7/17 15:25, Michal Hocko wrote: > On Wed 17-07-24 14:59:40, Miaohe Lin wrote: >> On 2024/7/17 14:18, Michal Hocko wrote: > [...] >>> Why does this need to be done through different modules? Why it cannot >>> be part of the memory-filure.c? >> >> This is a bold idea for me. :) I think it can be part of the memory-filure.c. >> So CONFIG_HWPOISON_INJECT should be removed from the world and then make >> hwpoison-inject default on when MEMORY_FAILURE is configured? > > I would start by moving code into mm/hwpoison-inject.c. Whether this > should also remove CONFIG_HWPOISON_INJECT is a different question. I > am not really sure a debugfs interface warrants a config option. Anyway > two things for two separate patches > Thanks for your suggestion. :) Will try to do it when I am free. Thanks. .
diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c index 7ecaa1900137..0b855cd3433a 100644 --- a/mm/hwpoison-inject.c +++ b/mm/hwpoison-inject.c @@ -64,10 +64,22 @@ static int hwpoison_unpoison(void *data, u64 val) DEFINE_DEBUGFS_ATTRIBUTE(hwpoison_fops, NULL, hwpoison_inject, "%lli\n"); DEFINE_DEBUGFS_ATTRIBUTE(unpoison_fops, NULL, hwpoison_unpoison, "%lli\n"); -static void __exit pfn_inject_exit(void) +static inline void reset_hwpoison_filter(void) { hwpoison_filter_enable = 0; + hwpoison_filter_dev_major = ~0U; + hwpoison_filter_dev_minor = ~0U; + hwpoison_filter_flags_mask = 0; + hwpoison_filter_flags_value = 0; +#ifdef CONFIG_MEMCG + hwpoison_filter_memcg = 0; +#endif +} + +static void __exit pfn_inject_exit(void) +{ debugfs_remove_recursive(hwpoison_dir); + reset_hwpoison_filter(); } static int __init pfn_inject_init(void)
When hwpoison_inject module is removed, hwpoison_filter_* parameters should be reset. Otherwise these parameters will have non-default values at next insmod time. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- v2: Add missing call to reset_hwpoison_filter() per Andrew. Thanks. --- mm/hwpoison-inject.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)