| Message ID | 20230411093632.822290-1-liushixin2@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Delay the initialization of zswap | expand |
Hi Shixing, On Tue, Apr 11, 2023 at 05:36:29PM +0800, Liu Shixin wrote: > In the initialization of zswap, about 18MB memory will be allocated for > zswap_pool. Since some users may not use zswap, the zswap_pool is wasted. > Save memory by delaying the initialization of zswap until enabled. Sorry I am late to this discussion. I notice you are already at V9. Anyway, I am curious how much of the 18MB was came from the zswap_pool alone and how much of it came from the other part of the initialization. If it is the zswap_pool alone, it means that we can have a smaller patch to get most of your 18M back. I also notice you move a lot of __init function back to normal functions. That would mean those functions wouldn't be able to drop after the initialization phase. That is another reason to move less of the initialization function. Chris
On 2023/5/4 8:11, Chris Li wrote: > Hi Shixing, > > On Tue, Apr 11, 2023 at 05:36:29PM +0800, Liu Shixin wrote: >> In the initialization of zswap, about 18MB memory will be allocated for >> zswap_pool. Since some users may not use zswap, the zswap_pool is wasted. >> Save memory by delaying the initialization of zswap until enabled. > Sorry I am late to this discussion. I notice you are already at V9. > Anyway, I am curious how much of the 18MB was came from the zswap_pool > alone and how much of it came from the other part of the initialization. > > If it is the zswap_pool alone, it means that we can have a smaller patch > to get most of your 18M back. You're right, the most came from zswap_pool. > > I also notice you move a lot of __init function back to normal functions. > That would mean those functions wouldn't be able to drop after the > initialization phase. That is another reason to move less of the initialization > function. Thanks for your advice. I've thought about it before, but I thought there is less impact for the size of kernel, so I didn't do it. > > Chris > > . >
On Thu, May 04, 2023 at 03:11:05PM +0800, Liu Shixin wrote: > > > > If it is the zswap_pool alone, it means that we can have a smaller patch > > to get most of your 18M back. > You're right, the most came from zswap_pool. Thanks for the confirmation. > > I also notice you move a lot of __init function back to normal functions. > > That would mean those functions wouldn't be able to drop after the > > initialization phase. That is another reason to move less of the initialization > > function. > Thanks for your advice. I've thought about it before, but I thought there is less impact > for the size of kernel, so I didn't do it. Let's first agree on the hypothetical patch that only delaying zswap_pool would have the benefit over V9 on: - smaller patch, less invasive. - less kernel text area due to more __init function got free after initialization. If we can reach that agreement, then we can discuss how we can get there. I think there is a possibility that the delay initialization of zswap_pool can fall into the "zswap_has_pool = false" case, so you don't need to have the initialization mutex. Simpler. I have my selfish reason as well. I have a much larger pending patch against the zswap code so the smaller patch would mean less conflict for me. I am guilty of giving this feedback late. If you come up with a V10, I will be glad to review it. Or, if you prefer, I can come up with the smaller patch for you to review as well. What do you say? Chris
On 2023/5/4 22:53, Chris Li wrote: > On Thu, May 04, 2023 at 03:11:05PM +0800, Liu Shixin wrote: >>> If it is the zswap_pool alone, it means that we can have a smaller patch >>> to get most of your 18M back. >> You're right, the most came from zswap_pool. > Thanks for the confirmation. > >>> I also notice you move a lot of __init function back to normal functions. >>> That would mean those functions wouldn't be able to drop after the >>> initialization phase. That is another reason to move less of the initialization >>> function. >> Thanks for your advice. I've thought about it before, but I thought there is less impact >> for the size of kernel, so I didn't do it. > Let's first agree on the hypothetical patch that only delaying zswap_pool would > have the benefit over V9 on: > - smaller patch, less invasive. > - less kernel text area due to more __init function got free after initialization. > > If we can reach that agreement, then we can discuss how we can get there. > > I think there is a possibility that the delay initialization of zswap_pool > can fall into the "zswap_has_pool = false" case, so you don't need to have > the initialization mutex. Simpler. > > I have my selfish reason as well. I have a much larger pending patch against > the zswap code so the smaller patch would mean less conflict for me. > > I am guilty of giving this feedback late. If you come up with a V10, I will be glad > to review it. Or, if you prefer, I can come up with the smaller patch for you > to review as well. What do you say? You can add a pre-patch to modify it before your patch. Thanks. > > Chris > > . >
Hi Shixin, On Mon, May 08, 2023 at 09:37:23AM +0800, Liu Shixin wrote: > > I am guilty of giving this feedback late. If you come up with a V10, I will be glad > > to review it. Or, if you prefer, I can come up with the smaller patch for you > > to review as well. What do you say? > You can add a pre-patch to modify it before your patch. Thanks. Will do. I notice your patch has already been merged, that is the only way to move forward anyway. I will CC you for review when I send the patch out. Chris
In the initialization of zswap, about 18MB memory will be allocated for zswap_pool. Since some users may not use zswap, the zswap_pool is wasted. Save memory by delaying the initialization of zswap until enabled. v8->v9: Fix some pattern problem suggested by Christoph. v7->v8: Do some cleanup. And remove the second patch in v7 which is unrelated to the initialization of zswap. v6->v7: Add two new patch[1,3] to cleanup the code. And cover zswap_init_* parameter by zswap_init_lock to protect against conflicts. v5->v6: Simplify the code and delete the patches about frontswap suggested by Christoph. Liu Shixin (3): mm/zswap: remove zswap_entry_cache_{create,destroy} helper function mm/zswap: replace zswap_init_{started/failed} with zswap_init_state mm/zswap: delay the initialization of zswap mm/zswap.c | 122 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 77 insertions(+), 45 deletions(-)