| Message ID | 20221229225100.22141-5-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fs/ufs: Replace kmap() with kmap_local_page | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, Dec 29, 2022 at 11:51:00PM +0100, Fabio M. De Francesco wrote: > kmap() is being deprecated in favor of kmap_local_page(). > > There are two main problems with kmap(): (1) It comes with an overhead as > the mapping space is restricted and protected by a global lock for > synchronization and (2) it also requires global TLB invalidation when the > kmap’s pool wraps and it might block when the mapping space is fully > utilized until a slot becomes available. > > With kmap_local_page() the mappings are per thread, CPU local, can take > page faults, and can be called from any context (including interrupts). > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > the tasks can be preempted and, when they are scheduled to run again, the > kernel virtual addresses are restored and still valid. > > The use of kmap_local_page() in fs/ufs is "safe" because (1) the kernel > virtual addresses are exclusively re-used by the thread which > established the mappings (i.e., thread locality is never violated) and (2) > the nestings of mappings and un-mappings are always stack based (LIFO). > > Therefore, replace kmap() with kmap_local_page() in fs/ufs. kunmap_local() > requires the mapping address, so return that address from ufs_get_page() > and use it as parameter for the second argument of ufs_put_page(). > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > Suggested-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > fs/ufs/dir.c | 72 +++++++++++++++++++++++++++++++++----------------- > fs/ufs/namei.c | 8 +++--- > fs/ufs/ufs.h | 2 +- > 3 files changed, 53 insertions(+), 29 deletions(-) > > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c > index 0bfd563ab0c2..8676a144e589 100644 > --- a/fs/ufs/dir.c > +++ b/fs/ufs/dir.c > @@ -61,9 +61,9 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len) > return err; > } > > -inline void ufs_put_page(struct page *page) > +inline void ufs_put_page(struct page *page, void *page_addr) > { > - kunmap(page); > + kunmap_local(page_addr); > put_page(page); > } > > @@ -76,7 +76,7 @@ ino_t ufs_inode_by_name(struct inode *dir, const struct qstr *qstr) > de = ufs_find_entry(dir, qstr, &page); > if (de) { > res = fs32_to_cpu(dir->i_sb, de->d_ino); > - ufs_put_page(page); > + ufs_put_page(page, de); > } > return res; > } > @@ -99,18 +99,17 @@ void ufs_set_link(struct inode *dir, struct ufs_dir_entry *de, > ufs_set_de_type(dir->i_sb, de, inode->i_mode); > > err = ufs_commit_chunk(page, pos, len); > - ufs_put_page(page); > + ufs_put_page(page, de); > if (update_times) > dir->i_mtime = dir->i_ctime = current_time(dir); > mark_inode_dirty(dir); > } > > > -static bool ufs_check_page(struct page *page) > +static bool ufs_check_page(struct page *page, char *kaddr) > { > struct inode *dir = page->mapping->host; > struct super_block *sb = dir->i_sb; > - char *kaddr = page_address(page); > unsigned offs, rec_len; > unsigned limit = PAGE_SIZE; > const unsigned chunk_mask = UFS_SB(sb)->s_uspi->s_dirblksize - 1; > @@ -185,23 +184,32 @@ static bool ufs_check_page(struct page *page) > return false; > } > > +/* > + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the > + * rules documented in kmap_local_page()/kunmap_local(). > + * > + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page() > + * and must be treated accordingly for nesting purposes. > + */ > static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **p) > { > + char *kaddr; > + > struct address_space *mapping = dir->i_mapping; > struct page *page = read_mapping_page(mapping, n, NULL); > if (!IS_ERR(page)) { > - kmap(page); > + kaddr = kmap_local_page(page); > if (unlikely(!PageChecked(page))) { > - if (!ufs_check_page(page)) > + if (!ufs_check_page(page, kaddr)) > goto fail; > } > *p = page; > - return page_address(page); > + return kaddr; > } > return ERR_CAST(page); > > fail: > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > return ERR_PTR(-EIO); > } > > @@ -227,6 +235,13 @@ ufs_next_entry(struct super_block *sb, struct ufs_dir_entry *p) > fs16_to_cpu(sb, p->d_reclen)); > } > > +/* > + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the > + * rules documented in kmap_local_page()/kunmap_local(). > + * > + * ufs_dotdot() acts as a call to ufs_get_page() and must be treated > + * accordingly for nesting purposes. > + */ > struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p) > { > struct ufs_dir_entry *de = ufs_get_page(dir, 0, p); > @@ -244,6 +259,11 @@ struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p) > * returns the page in which the entry was found, and the entry itself > * (as a parameter - res_dir). Page is returned mapped and unlocked. > * Entry is guaranteed to be valid. > + * > + * On Success ufs_put_page() should be called on *res_page. > + * > + * ufs_find_entry() acts as a call to ufs_get_page() and must be treated > + * accordingly for nesting purposes. > */ > struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr, > struct page **res_page) > @@ -282,7 +302,7 @@ struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr, > goto found; > de = ufs_next_entry(sb, de); > } > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > } > if (++n >= npages) > n = 0; > @@ -360,7 +380,7 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode) > de = (struct ufs_dir_entry *) ((char *) de + rec_len); > } > unlock_page(page); > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > } > BUG(); > return -EINVAL; > @@ -390,7 +410,7 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode) > mark_inode_dirty(dir); > /* OFFSET_CACHE */ > out_put: > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > return err; > out_unlock: > unlock_page(page); > @@ -468,13 +488,13 @@ ufs_readdir(struct file *file, struct dir_context *ctx) > ufs_get_de_namlen(sb, de), > fs32_to_cpu(sb, de->d_ino), > d_type)) { > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > return 0; > } > } > ctx->pos += fs16_to_cpu(sb, de->d_reclen); > } > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > } > return 0; > } > @@ -485,10 +505,15 @@ ufs_readdir(struct file *file, struct dir_context *ctx) > * previous entry. > */ > int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir, > - struct page * page) > + struct page *page) > { > struct super_block *sb = inode->i_sb; > - char *kaddr = page_address(page); > + /* > + * The "dir" dentry points somewhere in the same page whose we need the > + * address of; therefore, we can simply get the base address "kaddr" by > + * masking the previous with PAGE_MASK. > + */ > + char *kaddr = (char *)((unsigned long)dir & PAGE_MASK); > unsigned int from = offset_in_page(dir) & ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1); > unsigned int to = offset_in_page(dir) + fs16_to_cpu(sb, dir->d_reclen); > loff_t pos; > @@ -527,7 +552,7 @@ int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir, > inode->i_ctime = inode->i_mtime = current_time(inode); > mark_inode_dirty(inode); > out: > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > UFSD("EXIT\n"); > return err; > } > @@ -551,8 +576,7 @@ int ufs_make_empty(struct inode * inode, struct inode *dir) > goto fail; > } > > - kmap(page); > - base = (char*)page_address(page); > + base = kmap_local_page(page); > memset(base, 0, PAGE_SIZE); > > de = (struct ufs_dir_entry *) base; > @@ -569,7 +593,7 @@ int ufs_make_empty(struct inode * inode, struct inode *dir) > de->d_reclen = cpu_to_fs16(sb, chunk_size - UFS_DIR_REC_LEN(1)); > ufs_set_de_namlen(sb, de, 2); > strcpy (de->d_name, ".."); > - kunmap(page); > + kunmap_local(base); > > err = ufs_commit_chunk(page, 0, chunk_size); > fail: > @@ -585,9 +609,9 @@ int ufs_empty_dir(struct inode * inode) > struct super_block *sb = inode->i_sb; > struct page *page = NULL; > unsigned long i, npages = dir_pages(inode); > + char *kaddr; > > for (i = 0; i < npages; i++) { > - char *kaddr; > struct ufs_dir_entry *de; > > kaddr = ufs_get_page(inode, i, &page); > @@ -620,12 +644,12 @@ int ufs_empty_dir(struct inode * inode) > } > de = ufs_next_entry(sb, de); > } > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > } > return 1; > > not_empty: > - ufs_put_page(page); > + ufs_put_page(page, kaddr); > return 0; > } > > diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c > index 486b0f2e8b7a..7175d45e704c 100644 > --- a/fs/ufs/namei.c > +++ b/fs/ufs/namei.c > @@ -250,7 +250,7 @@ static int ufs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > struct inode *old_inode = d_inode(old_dentry); > struct inode *new_inode = d_inode(new_dentry); > struct page *dir_page = NULL; > - struct ufs_dir_entry * dir_de = NULL; > + struct ufs_dir_entry *dir_de = NULL; > struct page *old_page; > struct ufs_dir_entry *old_de; > int err = -ENOENT; > @@ -307,7 +307,7 @@ static int ufs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > if (old_dir != new_dir) > ufs_set_link(old_inode, dir_de, dir_page, new_dir, 0); > else { > - ufs_put_page(dir_page); > + ufs_put_page(dir_page, dir_de); > } > inode_dec_link_count(old_dir); > } > @@ -316,10 +316,10 @@ static int ufs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, > > out_dir: > if (dir_de) { > - ufs_put_page(dir_page); > + ufs_put_page(dir_page, dir_de); > } > out_old: > - ufs_put_page(old_page); > + ufs_put_page(old_page, old_de); > out: > return err; > } > diff --git a/fs/ufs/ufs.h b/fs/ufs/ufs.h > index f7ba8df25d03..942639e9a817 100644 > --- a/fs/ufs/ufs.h > +++ b/fs/ufs/ufs.h > @@ -98,7 +98,7 @@ extern struct ufs_cg_private_info * ufs_load_cylinder (struct super_block *, uns > extern void ufs_put_cylinder (struct super_block *, unsigned); > > /* dir.c */ > -extern void ufs_put_page(struct page *page); > +extern void ufs_put_page(struct page *page, void *vaddr); > extern const struct inode_operations ufs_dir_inode_operations; > extern int ufs_add_link (struct dentry *, struct inode *); > extern ino_t ufs_inode_by_name(struct inode *, const struct qstr *); > -- > 2.39.0 >
diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c index 0bfd563ab0c2..8676a144e589 100644 --- a/fs/ufs/dir.c +++ b/fs/ufs/dir.c @@ -61,9 +61,9 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len) return err; } -inline void ufs_put_page(struct page *page) +inline void ufs_put_page(struct page *page, void *page_addr) { - kunmap(page); + kunmap_local(page_addr); put_page(page); } @@ -76,7 +76,7 @@ ino_t ufs_inode_by_name(struct inode *dir, const struct qstr *qstr) de = ufs_find_entry(dir, qstr, &page); if (de) { res = fs32_to_cpu(dir->i_sb, de->d_ino); - ufs_put_page(page); + ufs_put_page(page, de); } return res; } @@ -99,18 +99,17 @@ void ufs_set_link(struct inode *dir, struct ufs_dir_entry *de, ufs_set_de_type(dir->i_sb, de, inode->i_mode); err = ufs_commit_chunk(page, pos, len); - ufs_put_page(page); + ufs_put_page(page, de); if (update_times) dir->i_mtime = dir->i_ctime = current_time(dir); mark_inode_dirty(dir); } -static bool ufs_check_page(struct page *page) +static bool ufs_check_page(struct page *page, char *kaddr) { struct inode *dir = page->mapping->host; struct super_block *sb = dir->i_sb; - char *kaddr = page_address(page); unsigned offs, rec_len; unsigned limit = PAGE_SIZE; const unsigned chunk_mask = UFS_SB(sb)->s_uspi->s_dirblksize - 1; @@ -185,23 +184,32 @@ static bool ufs_check_page(struct page *page) return false; } +/* + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the + * rules documented in kmap_local_page()/kunmap_local(). + * + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page() + * and must be treated accordingly for nesting purposes. + */ static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **p) { + char *kaddr; + struct address_space *mapping = dir->i_mapping; struct page *page = read_mapping_page(mapping, n, NULL); if (!IS_ERR(page)) { - kmap(page); + kaddr = kmap_local_page(page); if (unlikely(!PageChecked(page))) { - if (!ufs_check_page(page)) + if (!ufs_check_page(page, kaddr)) goto fail; } *p = page; - return page_address(page); + return kaddr; } return ERR_CAST(page); fail: - ufs_put_page(page); + ufs_put_page(page, kaddr); return ERR_PTR(-EIO); } @@ -227,6 +235,13 @@ ufs_next_entry(struct super_block *sb, struct ufs_dir_entry *p) fs16_to_cpu(sb, p->d_reclen)); } +/* + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the + * rules documented in kmap_local_page()/kunmap_local(). + * + * ufs_dotdot() acts as a call to ufs_get_page() and must be treated + * accordingly for nesting purposes. + */ struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p) { struct ufs_dir_entry *de = ufs_get_page(dir, 0, p); @@ -244,6 +259,11 @@ struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p) * returns the page in which the entry was found, and the entry itself * (as a parameter - res_dir). Page is returned mapped and unlocked. * Entry is guaranteed to be valid. + * + * On Success ufs_put_page() should be called on *res_page. + * + * ufs_find_entry() acts as a call to ufs_get_page() and must be treated + * accordingly for nesting purposes. */ struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr, struct page **res_page) @@ -282,7 +302,7 @@ struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr, goto found; de = ufs_next_entry(sb, de); } - ufs_put_page(page); + ufs_put_page(page, kaddr); } if (++n >= npages) n = 0; @@ -360,7 +380,7 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode) de = (struct ufs_dir_entry *) ((char *) de + rec_len); } unlock_page(page); - ufs_put_page(page); + ufs_put_page(page, kaddr); } BUG(); return -EINVAL; @@ -390,7 +410,7 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode) mark_inode_dirty(dir); /* OFFSET_CACHE */ out_put: - ufs_put_page(page); + ufs_put_page(page, kaddr); return err; out_unlock: unlock_page(page); @@ -468,13 +488,13 @@ ufs_readdir(struct file *file, struct dir_context *ctx) ufs_get_de_namlen(sb, de), fs32_to_cpu(sb, de->d_ino), d_type)) { - ufs_put_page(page); + ufs_put_page(page, kaddr); return 0; } } ctx->pos += fs16_to_cpu(sb, de->d_reclen); } - ufs_put_page(page); + ufs_put_page(page, kaddr); } return 0; } @@ -485,10 +505,15 @@ ufs_readdir(struct file *file, struct dir_context *ctx) * previous entry. */ int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir, - struct page * page) + struct page *page) { struct super_block *sb = inode->i_sb; - char *kaddr = page_address(page); + /* + * The "dir" dentry points somewhere in the same page whose we need the + * address of; therefore, we can simply get the base address "kaddr" by + * masking the previous with PAGE_MASK. + */ + char *kaddr = (char *)((unsigned long)dir & PAGE_MASK); unsigned int from = offset_in_page(dir) & ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1); unsigned int to = offset_in_page(dir) + fs16_to_cpu(sb, dir->d_reclen); loff_t pos; @@ -527,7 +552,7 @@ int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir, inode->i_ctime = inode->i_mtime = current_time(inode); mark_inode_dirty(inode); out: - ufs_put_page(page); + ufs_put_page(page, kaddr); UFSD("EXIT\n"); return err; } @@ -551,8 +576,7 @@ int ufs_make_empty(struct inode * inode, struct inode *dir) goto fail; } - kmap(page); - base = (char*)page_address(page); + base = kmap_local_page(page); memset(base, 0, PAGE_SIZE); de = (struct ufs_dir_entry *) base; @@ -569,7 +593,7 @@ int ufs_make_empty(struct inode * inode, struct inode *dir) de->d_reclen = cpu_to_fs16(sb, chunk_size - UFS_DIR_REC_LEN(1)); ufs_set_de_namlen(sb, de, 2); strcpy (de->d_name, ".."); - kunmap(page); + kunmap_local(base); err = ufs_commit_chunk(page, 0, chunk_size); fail: @@ -585,9 +609,9 @@ int ufs_empty_dir(struct inode * inode) struct super_block *sb = inode->i_sb; struct page *page = NULL; unsigned long i, npages = dir_pages(inode); + char *kaddr; for (i = 0; i < npages; i++) { - char *kaddr; struct ufs_dir_entry *de; kaddr = ufs_get_page(inode, i, &page); @@ -620,12 +644,12 @@ int ufs_empty_dir(struct inode * inode) } de = ufs_next_entry(sb, de); } - ufs_put_page(page); + ufs_put_page(page, kaddr); } return 1; not_empty: - ufs_put_page(page); + ufs_put_page(page, kaddr); return 0; } diff --git a/fs/ufs/namei.c b/fs/ufs/namei.c index 486b0f2e8b7a..7175d45e704c 100644 --- a/fs/ufs/namei.c +++ b/fs/ufs/namei.c @@ -250,7 +250,7 @@ static int ufs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, struct inode *old_inode = d_inode(old_dentry); struct inode *new_inode = d_inode(new_dentry); struct page *dir_page = NULL; - struct ufs_dir_entry * dir_de = NULL; + struct ufs_dir_entry *dir_de = NULL; struct page *old_page; struct ufs_dir_entry *old_de; int err = -ENOENT; @@ -307,7 +307,7 @@ static int ufs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, if (old_dir != new_dir) ufs_set_link(old_inode, dir_de, dir_page, new_dir, 0); else { - ufs_put_page(dir_page); + ufs_put_page(dir_page, dir_de); } inode_dec_link_count(old_dir); } @@ -316,10 +316,10 @@ static int ufs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, out_dir: if (dir_de) { - ufs_put_page(dir_page); + ufs_put_page(dir_page, dir_de); } out_old: - ufs_put_page(old_page); + ufs_put_page(old_page, old_de); out: return err; } diff --git a/fs/ufs/ufs.h b/fs/ufs/ufs.h index f7ba8df25d03..942639e9a817 100644 --- a/fs/ufs/ufs.h +++ b/fs/ufs/ufs.h @@ -98,7 +98,7 @@ extern struct ufs_cg_private_info * ufs_load_cylinder (struct super_block *, uns extern void ufs_put_cylinder (struct super_block *, unsigned); /* dir.c */ -extern void ufs_put_page(struct page *page); +extern void ufs_put_page(struct page *page, void *vaddr); extern const struct inode_operations ufs_dir_inode_operations; extern int ufs_add_link (struct dentry *, struct inode *); extern ino_t ufs_inode_by_name(struct inode *, const struct qstr *);
kmap() is being deprecated in favor of kmap_local_page(). There are two main problems with kmap(): (1) It comes with an overhead as the mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap’s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available. With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and still valid. The use of kmap_local_page() in fs/ufs is "safe" because (1) the kernel virtual addresses are exclusively re-used by the thread which established the mappings (i.e., thread locality is never violated) and (2) the nestings of mappings and un-mappings are always stack based (LIFO). Therefore, replace kmap() with kmap_local_page() in fs/ufs. kunmap_local() requires the mapping address, so return that address from ufs_get_page() and use it as parameter for the second argument of ufs_put_page(). Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Suggested-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- fs/ufs/dir.c | 72 +++++++++++++++++++++++++++++++++----------------- fs/ufs/namei.c | 8 +++--- fs/ufs/ufs.h | 2 +- 3 files changed, 53 insertions(+), 29 deletions(-)