| Message ID | 20240322083703.232364-6-alexs@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | transfer page to folio in KSM | expand |
On Fri, Mar 22, 2024 at 04:36:52PM +0800, alexs@kernel.org wrote: > -static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, > - struct ksm_stable_node **_stable_node, > - struct rb_root *root, > - bool prune_stale_stable_nodes) > +static void *stable_node_dup(struct ksm_stable_node **_stable_node_dup, > + struct ksm_stable_node **_stable_node, > + struct rb_root *root, > + bool prune_stale_stable_nodes) Do we really have to go through this void * stage? Also, please stop reindenting the arguments. I tend to just switch to two tabs, but lining them up with the opening bracket leads to extra churn. Either leave them alone for the entire series or switch _once_.
On 22.03.24 16:57, Matthew Wilcox wrote: > On Fri, Mar 22, 2024 at 04:36:52PM +0800, alexs@kernel.org wrote: >> -static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, >> - struct ksm_stable_node **_stable_node, >> - struct rb_root *root, >> - bool prune_stale_stable_nodes) >> +static void *stable_node_dup(struct ksm_stable_node **_stable_node_dup, >> + struct ksm_stable_node **_stable_node, >> + struct rb_root *root, >> + bool prune_stale_stable_nodes) > > Do we really have to go through this void * stage? > > Also, please stop reindenting the arguments. I tend to just switch to > two tabs, but lining them up with the opening bracket leads to extra > churn. Either leave them alone for the entire series or switch _once_. I wish the coding style would at least recommend something -- I know, different subsystems/files have their own rules. Nowadays, I prefer 2 tabs as well.
On 3/22/24 11:57 PM, Matthew Wilcox wrote: > On Fri, Mar 22, 2024 at 04:36:52PM +0800, alexs@kernel.org wrote: >> -static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, >> - struct ksm_stable_node **_stable_node, >> - struct rb_root *root, >> - bool prune_stale_stable_nodes) >> +static void *stable_node_dup(struct ksm_stable_node **_stable_node_dup, >> + struct ksm_stable_node **_stable_node, >> + struct rb_root *root, >> + bool prune_stale_stable_nodes) > > Do we really have to go through this void * stage? Hi Willy, Thank a lot for reminder. Yes, we could keep the 'struct page*' return value here. And so we don't need to change indent here. Thanks! > > Also, please stop reindenting the arguments. I tend to just switch to > two tabs, but lining them up with the opening bracket leads to extra > churn. Either leave them alone for the entire series or switch _once_. >
On 3/23/24 1:11 AM, David Hildenbrand wrote: > On 22.03.24 16:57, Matthew Wilcox wrote: >> On Fri, Mar 22, 2024 at 04:36:52PM +0800, alexs@kernel.org wrote: >>> -static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, >>> - struct ksm_stable_node **_stable_node, >>> - struct rb_root *root, >>> - bool prune_stale_stable_nodes) >>> +static void *stable_node_dup(struct ksm_stable_node **_stable_node_dup, >>> + struct ksm_stable_node **_stable_node, >>> + struct rb_root *root, >>> + bool prune_stale_stable_nodes) >> >> Do we really have to go through this void * stage? >> >> Also, please stop reindenting the arguments. I tend to just switch to >> two tabs, but lining them up with the opening bracket leads to extra >> churn. Either leave them alone for the entire series or switch _once_. > > I wish the coding style would at least recommend something -- I know, different subsystems/files have their own rules. Nowadays, I prefer 2 tabs as well. > Hi David, Thanks for comments, 2 tabs looks good. But just this file keep using more tabs. For the style alignment, let's keep them for now? Thanks
diff --git a/mm/ksm.c b/mm/ksm.c index b6ee2bc7646f..a1e382ecd501 100644 --- a/mm/ksm.c +++ b/mm/ksm.c @@ -1631,14 +1631,14 @@ bool is_page_sharing_candidate(struct ksm_stable_node *stable_node) return __is_page_sharing_candidate(stable_node, 0); } -static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, - struct ksm_stable_node **_stable_node, - struct rb_root *root, - bool prune_stale_stable_nodes) +static void *stable_node_dup(struct ksm_stable_node **_stable_node_dup, + struct ksm_stable_node **_stable_node, + struct rb_root *root, + bool prune_stale_stable_nodes) { struct ksm_stable_node *dup, *found = NULL, *stable_node = *_stable_node; struct hlist_node *hlist_safe; - struct page *_tree_page, *tree_page = NULL; + struct folio *folio, *tree_folio = NULL; int nr = 0; int found_rmap_hlist_len; @@ -1663,18 +1663,18 @@ static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, * stable_node parameter itself will be freed from * under us if it returns NULL. */ - _tree_page = get_ksm_page(dup, GET_KSM_PAGE_NOLOCK); - if (!_tree_page) + folio = ksm_get_folio(dup, GET_KSM_PAGE_NOLOCK); + if (!folio) continue; nr += 1; if (is_page_sharing_candidate(dup)) { if (!found || dup->rmap_hlist_len > found_rmap_hlist_len) { if (found) - put_page(tree_page); + folio_put(tree_folio); found = dup; found_rmap_hlist_len = found->rmap_hlist_len; - tree_page = _tree_page; + tree_folio = folio; /* skip put_page for found dup */ if (!prune_stale_stable_nodes) @@ -1682,7 +1682,7 @@ static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, continue; } } - put_page(_tree_page); + folio_put(folio); } if (found) { @@ -1747,7 +1747,7 @@ static struct page *stable_node_dup(struct ksm_stable_node **_stable_node_dup, } *_stable_node_dup = found; - return tree_page; + return tree_folio; } static struct ksm_stable_node *stable_node_dup_any(struct ksm_stable_node *stable_node,