Messages in this thread | | | Subject | Re: [PATCH 10/10] mm/migrate.c: call detach_page_private to cleanup code | From | Guoqing Jiang <> | Date | Tue, 19 May 2020 09:35:59 +0200 |
| |
On 5/19/20 7:12 AM, Andrew Morton wrote: > On Sun, 17 May 2020 23:47:18 +0200 Guoqing Jiang <guoqing.jiang@cloud.ionos.com> wrote: > >> We can cleanup code a little by call detach_page_private here. >> >> ... >> >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -804,10 +804,7 @@ static int __buffer_migrate_page(struct address_space *mapping, >> if (rc != MIGRATEPAGE_SUCCESS) >> goto unlock_buffers; >> >> - ClearPagePrivate(page); >> - set_page_private(newpage, page_private(page)); >> - set_page_private(page, 0); >> - put_page(page); >> + set_page_private(newpage, detach_page_private(page)); >> get_page(newpage); >> >> bh = head; > mm/migrate.c: In function '__buffer_migrate_page': > ./include/linux/mm_types.h:243:52: warning: assignment makes integer from pointer without a cast [-Wint-conversion] > #define set_page_private(page, v) ((page)->private = (v)) > ^ > mm/migrate.c:800:2: note: in expansion of macro 'set_page_private' > set_page_private(newpage, detach_page_private(page)); > ^~~~~~~~~~~~~~~~ > > The fact that set_page_private(detach_page_private()) generates a type > mismatch warning seems deeply wrong, surely. > > Please let's get the types sorted out - either unsigned long or void *, > not half-one and half-the other. Whatever needs the least typecasting > at callsites, I suggest.
Sorry about that, I should notice the warning before. I will double check if other places need the typecast or not, then send a new version.
> And can we please implement set_page_private() and page_private() with > inlined C code? There is no need for these to be macros.
Just did a quick change.
-#define page_private(page) ((page)->private) -#define set_page_private(page, v) ((page)->private = (v)) +static inline unsigned long page_private(struct page *page) +{ + return page->private; +} + +static inline void set_page_private(struct page *page, unsigned long priv_data) +{ + page->private = priv_data; +}
Then I get error like.
fs/erofs/zdata.h: In function ‘z_erofs_onlinepage_index’: fs/erofs/zdata.h:126:8: error: lvalue required as unary ‘&’ operand u.v = &page_private(page); ^
I guess it is better to keep page_private as macro, please correct me in case I missed something.
Thanks, Guoqing
| |