| Message ID | 20190311010744.5862-1-tobin@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | mm: Use slab_list list_head instead of lru | expand |
On Mon, Mar 11, 2019 at 12:07:40PM +1100, Tobin C. Harding wrote: > Currently the slab allocators (ab)use the struct page 'lru' list_head. > We have a list head for slab allocators to use, 'slab_list'. > > Clean up all three allocators by using the 'slab_list' list_head instead > of overloading the 'lru' list_head. > > Initial patch makes no code changes, adds comments to #endif statements. > > Final 3 patches do changes as a patch per allocator, tested by building > and booting (in Qemu) after configuring kernel to use appropriate > allocator. Also build and boot with debug options enabled (for slab > and slub). Hi Tobin! The patchset looks good to me, however I'd add some clarifications why switching from lru to slab_list is safe. My understanding is that the slab_list fields isn't currently in use, but it's not that obvious that putting slab_list and next/pages/pobjects fields into a union is safe (for the slub case). Please, add a clarification/comment. For patches 1, 3 and 4: Reviewed-by: Roman Gushchin <guro@fb.com> Thanks!
On Mon, Mar 11, 2019 at 08:49:23PM +0000, Roman Gushchin wrote: > The patchset looks good to me, however I'd add some clarifications > why switching from lru to slab_list is safe. > > My understanding is that the slab_list fields isn't currently in use, > but it's not that obvious that putting slab_list and next/pages/pobjects > fields into a union is safe (for the slub case). It's already in a union. struct page { union { struct { /* Page cache and anonymous pages */ struct list_head lru; ... struct { /* slab, slob and slub */ union { struct list_head slab_list; /* uses lru */ struct { /* Partial pages */ struct page *next; slab_list and lru are in the same bits. Once this patch set is in, we can remove the enigmatic 'uses lru' comment that I added.
On Mon, Mar 11, 2019 at 04:16:33PM -0700, Matthew Wilcox wrote: > On Mon, Mar 11, 2019 at 08:49:23PM +0000, Roman Gushchin wrote: > > The patchset looks good to me, however I'd add some clarifications > > why switching from lru to slab_list is safe. > > > > My understanding is that the slab_list fields isn't currently in use, > > but it's not that obvious that putting slab_list and next/pages/pobjects > > fields into a union is safe (for the slub case). > > It's already in a union. > > struct page { > union { > struct { /* Page cache and anonymous pages */ > struct list_head lru; > ... > struct { /* slab, slob and slub */ > union { > struct list_head slab_list; /* uses lru */ > struct { /* Partial pages */ > struct page *next; > > slab_list and lru are in the same bits. Once this patch set is in, > we can remove the enigmatic 'uses lru' comment that I added. Ah, perfect, thanks! Makes total sense then. Tobin, can you, please, add a note to the commit message? With the note: Reviewed-by: Roman Gushchin <guro@fb.com> Thank you!
On Mon, Mar 11, 2019 at 04:16:33PM -0700, Matthew Wilcox wrote: > On Mon, Mar 11, 2019 at 08:49:23PM +0000, Roman Gushchin wrote: > > The patchset looks good to me, however I'd add some clarifications > > why switching from lru to slab_list is safe. > > > > My understanding is that the slab_list fields isn't currently in use, > > but it's not that obvious that putting slab_list and next/pages/pobjects > > fields into a union is safe (for the slub case). > > It's already in a union. > > struct page { > union { > struct { /* Page cache and anonymous pages */ > struct list_head lru; > ... > struct { /* slab, slob and slub */ > union { > struct list_head slab_list; /* uses lru */ > struct { /* Partial pages */ > struct page *next; > > slab_list and lru are in the same bits. Once this patch set is in, > we can remove the enigmatic 'uses lru' comment that I added. Funny you should say this, I came to me today while daydreaming that I should have removed that comment :) I'll remove it in v2. thanks, Tobin.
On Tue, Mar 12, 2019 at 12:22:23AM +0000, Roman Gushchin wrote: > On Mon, Mar 11, 2019 at 04:16:33PM -0700, Matthew Wilcox wrote: > > On Mon, Mar 11, 2019 at 08:49:23PM +0000, Roman Gushchin wrote: > > > The patchset looks good to me, however I'd add some clarifications > > > why switching from lru to slab_list is safe. > > > > > > My understanding is that the slab_list fields isn't currently in use, > > > but it's not that obvious that putting slab_list and next/pages/pobjects > > > fields into a union is safe (for the slub case). > > > > It's already in a union. > > > > struct page { > > union { > > struct { /* Page cache and anonymous pages */ > > struct list_head lru; > > ... > > struct { /* slab, slob and slub */ > > union { > > struct list_head slab_list; /* uses lru */ > > struct { /* Partial pages */ > > struct page *next; > > > > slab_list and lru are in the same bits. Once this patch set is in, > > we can remove the enigmatic 'uses lru' comment that I added. > > Ah, perfect, thanks! Makes total sense then. > > Tobin, can you, please, add a note to the commit message? > With the note: > Reviewed-by: Roman Gushchin <guro@fb.com> Thanks for the review Roman, will add tag to v2. Tobin.
On Tue, Mar 12, 2019 at 12:22:23AM +0000, Roman Gushchin wrote: > On Mon, Mar 11, 2019 at 04:16:33PM -0700, Matthew Wilcox wrote: > > On Mon, Mar 11, 2019 at 08:49:23PM +0000, Roman Gushchin wrote: > > > The patchset looks good to me, however I'd add some clarifications > > > why switching from lru to slab_list is safe. > > > > > > My understanding is that the slab_list fields isn't currently in use, > > > but it's not that obvious that putting slab_list and next/pages/pobjects > > > fields into a union is safe (for the slub case). > > > > It's already in a union. > > > > struct page { > > union { > > struct { /* Page cache and anonymous pages */ > > struct list_head lru; > > ... > > struct { /* slab, slob and slub */ > > union { > > struct list_head slab_list; /* uses lru */ > > struct { /* Partial pages */ > > struct page *next; > > > > slab_list and lru are in the same bits. Once this patch set is in, > > we can remove the enigmatic 'uses lru' comment that I added. > > Ah, perfect, thanks! Makes total sense then. > > Tobin, can you, please, add a note to the commit message? > With the note: > Reviewed-by: Roman Gushchin <guro@fb.com> Awesome, thanks. That's for all 4 patches or excluding 2? thanks, Tobin.
On Tue, Mar 12, 2019 at 12:05:54PM +1100, Tobin C. Harding wrote: > > slab_list and lru are in the same bits. Once this patch set is in, > > we can remove the enigmatic 'uses lru' comment that I added. > > Funny you should say this, I came to me today while daydreaming that I > should have removed that comment :) > > I'll remove it in v2. That's great. BTW, something else you could do to verify this patch set is check that the object file is unchanged before/after the patch. I tend to use 'objdump -dr' to before.s and after.s and use 'diff' to compare the two.
On Mon, Mar 11, 2019 at 07:38:28PM -0700, Matthew Wilcox wrote: > On Tue, Mar 12, 2019 at 12:05:54PM +1100, Tobin C. Harding wrote: > > > slab_list and lru are in the same bits. Once this patch set is in, > > > we can remove the enigmatic 'uses lru' comment that I added. > > > > Funny you should say this, I came to me today while daydreaming that I > > should have removed that comment :) > > > > I'll remove it in v2. > > That's great. BTW, something else you could do to verify this patch > set is check that the object file is unchanged before/after the patch. > I tend to use 'objdump -dr' to before.s and after.s and use 'diff' > to compare the two. Oh cool, I didn't know to do that. I'm not super familiar with the use of unions having never had need to use one myself so any other union related tips you think of please share. thanks, Tobin.
On Tue, Mar 12, 2019 at 01:01:53PM +1100, Tobin C. Harding wrote: > On Tue, Mar 12, 2019 at 12:22:23AM +0000, Roman Gushchin wrote: > > On Mon, Mar 11, 2019 at 04:16:33PM -0700, Matthew Wilcox wrote: > > > On Mon, Mar 11, 2019 at 08:49:23PM +0000, Roman Gushchin wrote: > > > > The patchset looks good to me, however I'd add some clarifications > > > > why switching from lru to slab_list is safe. > > > > > > > > My understanding is that the slab_list fields isn't currently in use, > > > > but it's not that obvious that putting slab_list and next/pages/pobjects > > > > fields into a union is safe (for the slub case). > > > > > > It's already in a union. > > > > > > struct page { > > > union { > > > struct { /* Page cache and anonymous pages */ > > > struct list_head lru; > > > ... > > > struct { /* slab, slob and slub */ > > > union { > > > struct list_head slab_list; /* uses lru */ > > > struct { /* Partial pages */ > > > struct page *next; > > > > > > slab_list and lru are in the same bits. Once this patch set is in, > > > we can remove the enigmatic 'uses lru' comment that I added. > > > > Ah, perfect, thanks! Makes total sense then. > > > > Tobin, can you, please, add a note to the commit message? > > With the note: > > Reviewed-by: Roman Gushchin <guro@fb.com> > > Awesome, thanks. That's for all 4 patches or excluding 2? To all 4, given that you'll add some explanations to the commit message. Thanks!
On Mon, Mar 11, 2019 at 07:38:28PM -0700, Matthew Wilcox wrote: > On Tue, Mar 12, 2019 at 12:05:54PM +1100, Tobin C. Harding wrote: > > > slab_list and lru are in the same bits. Once this patch set is in, > > > we can remove the enigmatic 'uses lru' comment that I added. > > > > Funny you should say this, I came to me today while daydreaming that I > > should have removed that comment :) > > > > I'll remove it in v2. > > That's great. BTW, something else you could do to verify this patch > set is check that the object file is unchanged before/after the patch. > I tend to use 'objdump -dr' to before.s and after.s and use 'diff' > to compare the two. Btw, is it guaranteed that the object file will not change? I was about to recommend the same, but was not sure, if such change can cause gcc to generate a *slightly* different obj code. Thanks!