| Message ID | 20241115150120.3280-3-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | TTM shrinker helpers and xe buffer object shrinker | expand |
Am 15.11.24 um 16:01 schrieb Thomas Hellström: > Provide a standalone shmem backup implementation. > Given the ttm_backup interface, this could > later on be extended to providing other backup > implementation than shmem, with one use-case being > GPU swapout to a user-provided fd. > > v5: > - Fix a UAF. (kernel test robot, Dan Carptenter) > v6: > - Rename ttm_backup_shmem_copy_page() function argument > (Matthew Brost) > - Add some missing documentation > v8: > - Use folio_file_page to get to the page we want to writeback > instead of using the first page of the folio. > v13: > - Remove the base class abstraction (Christian König) > - Include ttm_backup_bytes_avail(). > v14: > - Fix kerneldoc for ttm_backup_bytes_avail() (0-day) > - Work around casting of __randomize_layout struct pointer (0-day) > > Cc: Christian König <christian.koenig@amd.com> > Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: <dri-devel@lists.freedesktop.org> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Reviewed-by: Matthew Brost <matthew.brost@intel.com> #v13 > --- > drivers/gpu/drm/ttm/Makefile | 2 +- > drivers/gpu/drm/ttm/ttm_backup.c | 204 +++++++++++++++++++++++++++++++ > include/drm/ttm/ttm_backup.h | 74 +++++++++++ > 3 files changed, 279 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/ttm/ttm_backup.c > create mode 100644 include/drm/ttm/ttm_backup.h > > diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile > index dad298127226..40d07a35293a 100644 > --- a/drivers/gpu/drm/ttm/Makefile > +++ b/drivers/gpu/drm/ttm/Makefile > @@ -4,7 +4,7 @@ > > ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \ > ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o ttm_pool.o \ > - ttm_device.o ttm_sys_manager.o > + ttm_device.o ttm_sys_manager.o ttm_backup.o > ttm-$(CONFIG_AGP) += ttm_agp_backend.o > > obj-$(CONFIG_DRM_TTM) += ttm.o > diff --git a/drivers/gpu/drm/ttm/ttm_backup.c b/drivers/gpu/drm/ttm/ttm_backup.c > new file mode 100644 > index 000000000000..bf16bb0c594e > --- /dev/null > +++ b/drivers/gpu/drm/ttm/ttm_backup.c > @@ -0,0 +1,204 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#include <drm/ttm/ttm_backup.h> > +#include <linux/page-flags.h> > +#include <linux/swap.h> > + > +/* > + * Casting from randomized struct file * to struct ttm_backup * is fine since > + * struct ttm_backup is never defined nor dereferenced. > + */ > +static struct file *ttm_backup_to_file(struct ttm_backup *backup) Do I get it right that struct ttm_backup is never really defined? What purpose does that have? > +{ > + return (void *)backup; > +} > + > +static struct ttm_backup *ttm_file_to_backup(struct file *file) > +{ > + return (void *)file; > +} > + > +/* > + * Need to map shmem indices to handle since a handle value > + * of 0 means error, following the swp_entry_t convention. > + */ > +static unsigned long ttm_backup_shmem_idx_to_handle(pgoff_t idx) > +{ > + return (unsigned long)idx + 1; > +} > + > +static pgoff_t ttm_backup_handle_to_shmem_idx(pgoff_t handle) > +{ > + return handle - 1; > +} > + > +/** > + * ttm_backup_drop() - release memory associated with a handle > + * @backup: The struct backup pointer used to obtain the handle > + * @handle: The handle obtained from the @backup_page function. > + */ > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle) > +{ > + loff_t start = ttm_backup_handle_to_shmem_idx(handle); > + > + start <<= PAGE_SHIFT; > + shmem_truncate_range(file_inode(ttm_backup_to_file(backup)), start, > + start + PAGE_SIZE - 1); > +} > + > +/** > + * ttm_backup_copy_page() - Copy the contents of a previously backed > + * up page > + * @backup: The struct backup pointer used to back up the page. > + * @dst: The struct page to copy into. > + * @handle: The handle returned when the page was backed up. > + * @intr: Try to perform waits interruptable or at least killable. > + * > + * Return: 0 on success, Negative error code on failure, notably > + * -EINTR if @intr was set to true and a signal is pending. > + */ > +int ttm_backup_copy_page(struct ttm_backup *backup, struct page *dst, > + pgoff_t handle, bool intr) > +{ > + struct file *filp = ttm_backup_to_file(backup); > + struct address_space *mapping = filp->f_mapping; > + struct folio *from_folio; > + pgoff_t idx = ttm_backup_handle_to_shmem_idx(handle); > + > + from_folio = shmem_read_folio(mapping, idx); > + if (IS_ERR(from_folio)) > + return PTR_ERR(from_folio); > + > + copy_highpage(dst, folio_file_page(from_folio, idx)); > + folio_put(from_folio); > + > + return 0; > +} > + > +/** > + * ttm_backup_backup_page() - Backup a page > + * @backup: The struct backup pointer to use. > + * @page: The page to back up. > + * @writeback: Whether to perform immediate writeback of the page. > + * This may have performance implications. > + * @idx: A unique integer for each page and each struct backup. > + * This allows the backup implementation to avoid managing > + * its address space separately. > + * @page_gfp: The gfp value used when the page was allocated. > + * This is used for accounting purposes. > + * @alloc_gfp: The gpf to be used when allocating memory. Typo: gfp instead of gpf. > + * > + * Context: If called from reclaim context, the caller needs to > + * assert that the shrinker gfp has __GFP_FS set, to avoid > + * deadlocking on lock_page(). If @writeback is set to true and > + * called from reclaim context, the caller also needs to assert > + * that the shrinker gfp has __GFP_IO set, since without it, > + * we're not allowed to start backup IO. > + * > + * Return: A handle on success. 0 on failure. > + * (This is following the swp_entry_t convention). > + * > + * Note: This function could be extended to back up a folio and > + * implementations would then split the folio internally if needed. > + * Drawback is that the caller would then have to keep track of > + * the folio size- and usage. > + */ > +unsigned long > +ttm_backup_backup_page(struct ttm_backup *backup, struct page *page, > + bool writeback, pgoff_t idx, gfp_t page_gfp, > + gfp_t alloc_gfp) > +{ > + struct file *filp = ttm_backup_to_file(backup); > + struct address_space *mapping = filp->f_mapping; > + unsigned long handle = 0; > + struct folio *to_folio; > + int ret; > + > + to_folio = shmem_read_folio_gfp(mapping, idx, alloc_gfp); > + if (IS_ERR(to_folio)) > + return handle; Just that I sleep better: This can never return a folio larger than a page, doesn't it? Apart from those background questions looks good to me. Regards, Christian. > + > + folio_mark_accessed(to_folio); > + folio_lock(to_folio); > + folio_mark_dirty(to_folio); > + copy_highpage(folio_file_page(to_folio, idx), page); > + handle = ttm_backup_shmem_idx_to_handle(idx); > + > + if (writeback && !folio_mapped(to_folio) && > + folio_clear_dirty_for_io(to_folio)) { > + struct writeback_control wbc = { > + .sync_mode = WB_SYNC_NONE, > + .nr_to_write = SWAP_CLUSTER_MAX, > + .range_start = 0, > + .range_end = LLONG_MAX, > + .for_reclaim = 1, > + }; > + folio_set_reclaim(to_folio); > + ret = mapping->a_ops->writepage(folio_file_page(to_folio, idx), &wbc); > + if (!folio_test_writeback(to_folio)) > + folio_clear_reclaim(to_folio); > + /* If writepage succeeds, it unlocks the folio */ > + if (ret) > + folio_unlock(to_folio); > + } else { > + folio_unlock(to_folio); > + } > + > + folio_put(to_folio); > + > + return handle; > +} > + > +/** > + * ttm_backup_fini() - Free the struct backup resources after last use. > + * @backup: Pointer to the struct backup whose resources to free. > + * > + * After a call to this function, it's illegal to use the @backup pointer. > + */ > +void ttm_backup_fini(struct ttm_backup *backup) > +{ > + fput(ttm_backup_to_file(backup)); > +} > + > +/** > + * ttm_backup_bytes_avail() - Report the approximate number of bytes of backup space > + * left for backup. > + * > + * This function is intended also for driver use to indicate whether a > + * backup attempt is meaningful. > + * > + * Return: An approximate size of backup space available. > + */ > +u64 ttm_backup_bytes_avail(void) > +{ > + /* > + * The idea behind backing up to shmem is that shmem objects may > + * eventually be swapped out. So no point swapping out if there > + * is no or low swap-space available. But the accuracy of this > + * number also depends on shmem actually swapping out backed-up > + * shmem objects without too much buffering. > + */ > + return (u64)get_nr_swap_pages() << PAGE_SHIFT; > +} > +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); > + > +/** > + * ttm_backup_shmem_create() - Create a shmem-based struct backup. > + * @size: The maximum size (in bytes) to back up. > + * > + * Create a backup utilizing shmem objects. > + * > + * Return: A pointer to a struct ttm_backup on success, > + * an error pointer on error. > + */ > +struct ttm_backup *ttm_backup_shmem_create(loff_t size) > +{ > + struct file *filp; > + > + filp = shmem_file_setup("ttm shmem backup", size, 0); > + > + return ttm_file_to_backup(filp); > +} > diff --git a/include/drm/ttm/ttm_backup.h b/include/drm/ttm/ttm_backup.h > new file mode 100644 > index 000000000000..20609da7e281 > --- /dev/null > +++ b/include/drm/ttm/ttm_backup.h > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#ifndef _TTM_BACKUP_H_ > +#define _TTM_BACKUP_H_ > + > +#include <linux/mm_types.h> > +#include <linux/shmem_fs.h> > + > +struct ttm_backup; > + > +/** > + * ttm_backup_handle_to_page_ptr() - Convert handle to struct page pointer > + * @handle: The handle to convert. > + * > + * Converts an opaque handle received from the > + * struct ttm_backoup_ops::backup_page() function to an (invalid) > + * struct page pointer suitable for a struct page array. > + * > + * Return: An (invalid) struct page pointer. > + */ > +static inline struct page * > +ttm_backup_handle_to_page_ptr(unsigned long handle) > +{ > + return (struct page *)(handle << 1 | 1); > +} > + > +/** > + * ttm_backup_page_ptr_is_handle() - Whether a struct page pointer is a handle > + * @page: The struct page pointer to check. > + * > + * Return: true if the struct page pointer is a handld returned from > + * ttm_backup_handle_to_page_ptr(). False otherwise. > + */ > +static inline bool ttm_backup_page_ptr_is_handle(const struct page *page) > +{ > + return (unsigned long)page & 1; > +} > + > +/** > + * ttm_backup_page_ptr_to_handle() - Convert a struct page pointer to a handle > + * @page: The struct page pointer to convert > + * > + * Return: The handle that was previously used in > + * ttm_backup_handle_to_page_ptr() to obtain a struct page pointer, suitable > + * for use as argument in the struct ttm_backup_ops drop() or > + * copy_backed_up_page() functions. > + */ > +static inline unsigned long > +ttm_backup_page_ptr_to_handle(const struct page *page) > +{ > + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); > + return (unsigned long)page >> 1; > +} > + > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle); > + > +int ttm_backup_copy_page(struct ttm_backup *backup, struct page *dst, > + pgoff_t handle, bool intr); > + > +unsigned long > +ttm_backup_backup_page(struct ttm_backup *backup, struct page *page, > + bool writeback, pgoff_t idx, gfp_t page_gfp, > + gfp_t alloc_gfp); > + > +void ttm_backup_fini(struct ttm_backup *backup); > + > +u64 ttm_backup_bytes_avail(void); > + > +struct ttm_backup *ttm_backup_shmem_create(loff_t size); > + > +#endif
On Tue, 2024-11-19 at 14:40 +0100, Christian König wrote: > Am 15.11.24 um 16:01 schrieb Thomas Hellström: > > Provide a standalone shmem backup implementation. > > Given the ttm_backup interface, this could > > later on be extended to providing other backup > > implementation than shmem, with one use-case being > > GPU swapout to a user-provided fd. > > > > v5: > > - Fix a UAF. (kernel test robot, Dan Carptenter) > > v6: > > - Rename ttm_backup_shmem_copy_page() function argument > > (Matthew Brost) > > - Add some missing documentation > > v8: > > - Use folio_file_page to get to the page we want to writeback > > instead of using the first page of the folio. > > v13: > > - Remove the base class abstraction (Christian König) > > - Include ttm_backup_bytes_avail(). > > v14: > > - Fix kerneldoc for ttm_backup_bytes_avail() (0-day) > > - Work around casting of __randomize_layout struct pointer (0-day) > > > > Cc: Christian König <christian.koenig@amd.com> > > Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com> > > Cc: Matthew Brost <matthew.brost@intel.com> > > Cc: <dri-devel@lists.freedesktop.org> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Reviewed-by: Matthew Brost <matthew.brost@intel.com> #v13 > > --- > > drivers/gpu/drm/ttm/Makefile | 2 +- > > drivers/gpu/drm/ttm/ttm_backup.c | 204 > > +++++++++++++++++++++++++++++++ > > include/drm/ttm/ttm_backup.h | 74 +++++++++++ > > 3 files changed, 279 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpu/drm/ttm/ttm_backup.c > > create mode 100644 include/drm/ttm/ttm_backup.h > > > > diff --git a/drivers/gpu/drm/ttm/Makefile > > b/drivers/gpu/drm/ttm/Makefile > > index dad298127226..40d07a35293a 100644 > > --- a/drivers/gpu/drm/ttm/Makefile > > +++ b/drivers/gpu/drm/ttm/Makefile > > @@ -4,7 +4,7 @@ > > > > ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o > > \ > > ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o > > ttm_pool.o \ > > - ttm_device.o ttm_sys_manager.o > > + ttm_device.o ttm_sys_manager.o ttm_backup.o > > ttm-$(CONFIG_AGP) += ttm_agp_backend.o > > > > obj-$(CONFIG_DRM_TTM) += ttm.o > > diff --git a/drivers/gpu/drm/ttm/ttm_backup.c > > b/drivers/gpu/drm/ttm/ttm_backup.c > > new file mode 100644 > > index 000000000000..bf16bb0c594e > > --- /dev/null > > +++ b/drivers/gpu/drm/ttm/ttm_backup.c > > @@ -0,0 +1,204 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2024 Intel Corporation > > + */ > > + > > +#include <drm/ttm/ttm_backup.h> > > +#include <linux/page-flags.h> > > +#include <linux/swap.h> > > + > > +/* > > + * Casting from randomized struct file * to struct ttm_backup * is > > fine since > > + * struct ttm_backup is never defined nor dereferenced. > > + */ > > +static struct file *ttm_backup_to_file(struct ttm_backup *backup) > > Do I get it right that struct ttm_backup is never really defined? Yes. > What > purpose does that have? It's to make the struct ttm_backup opaque to the users of the ttm_backup interface, so that the implementation doesn't have to worry about the user making illegal assumptions about the implementation. > > +{ > > + return (void *)backup; > > +} > > + > > +static struct ttm_backup *ttm_file_to_backup(struct file *file) > > +{ > > + return (void *)file; > > +} > > + > > +/* > > + * Need to map shmem indices to handle since a handle value > > + * of 0 means error, following the swp_entry_t convention. > > + */ > > +static unsigned long ttm_backup_shmem_idx_to_handle(pgoff_t idx) > > +{ > > + return (unsigned long)idx + 1; > > +} > > + > > +static pgoff_t ttm_backup_handle_to_shmem_idx(pgoff_t handle) > > +{ > > + return handle - 1; > > +} > > + > > +/** > > + * ttm_backup_drop() - release memory associated with a handle > > + * @backup: The struct backup pointer used to obtain the handle > > + * @handle: The handle obtained from the @backup_page function. > > + */ > > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle) > > +{ > > + loff_t start = ttm_backup_handle_to_shmem_idx(handle); > > + > > + start <<= PAGE_SHIFT; > > + shmem_truncate_range(file_inode(ttm_backup_to_file(backup) > > ), start, > > + start + PAGE_SIZE - 1); > > +} > > + > > +/** > > + * ttm_backup_copy_page() - Copy the contents of a previously > > backed > > + * up page > > + * @backup: The struct backup pointer used to back up the page. > > + * @dst: The struct page to copy into. > > + * @handle: The handle returned when the page was backed up. > > + * @intr: Try to perform waits interruptable or at least killable. > > + * > > + * Return: 0 on success, Negative error code on failure, notably > > + * -EINTR if @intr was set to true and a signal is pending. > > + */ > > +int ttm_backup_copy_page(struct ttm_backup *backup, struct page > > *dst, > > + pgoff_t handle, bool intr) > > +{ > > + struct file *filp = ttm_backup_to_file(backup); > > + struct address_space *mapping = filp->f_mapping; > > + struct folio *from_folio; > > + pgoff_t idx = ttm_backup_handle_to_shmem_idx(handle); > > + > > + from_folio = shmem_read_folio(mapping, idx); > > + if (IS_ERR(from_folio)) > > + return PTR_ERR(from_folio); > > + > > + copy_highpage(dst, folio_file_page(from_folio, idx)); > > + folio_put(from_folio); > > + > > + return 0; > > +} > > + > > +/** > > + * ttm_backup_backup_page() - Backup a page > > + * @backup: The struct backup pointer to use. > > + * @page: The page to back up. > > + * @writeback: Whether to perform immediate writeback of the page. > > + * This may have performance implications. > > + * @idx: A unique integer for each page and each struct backup. > > + * This allows the backup implementation to avoid managing > > + * its address space separately. > > + * @page_gfp: The gfp value used when the page was allocated. > > + * This is used for accounting purposes. > > + * @alloc_gfp: The gpf to be used when allocating memory. > > Typo: gfp instead of gpf. Sure. > > > + * > > + * Context: If called from reclaim context, the caller needs to > > + * assert that the shrinker gfp has __GFP_FS set, to avoid > > + * deadlocking on lock_page(). If @writeback is set to true and > > + * called from reclaim context, the caller also needs to assert > > + * that the shrinker gfp has __GFP_IO set, since without it, > > + * we're not allowed to start backup IO. > > + * > > + * Return: A handle on success. 0 on failure. > > + * (This is following the swp_entry_t convention). > > + * > > + * Note: This function could be extended to back up a folio and > > + * implementations would then split the folio internally if > > needed. > > + * Drawback is that the caller would then have to keep track of > > + * the folio size- and usage. > > + */ > > +unsigned long > > +ttm_backup_backup_page(struct ttm_backup *backup, struct page > > *page, > > + bool writeback, pgoff_t idx, gfp_t > > page_gfp, > > + gfp_t alloc_gfp) > > +{ > > + struct file *filp = ttm_backup_to_file(backup); > > + struct address_space *mapping = filp->f_mapping; > > + unsigned long handle = 0; > > + struct folio *to_folio; > > + int ret; > > + > > + to_folio = shmem_read_folio_gfp(mapping, idx, alloc_gfp); > > + if (IS_ERR(to_folio)) > > + return handle; > > Just that I sleep better: This can never return a folio larger than a > page, doesn't it? The interface definitely allows for returning larger folios, but the individual page in the folio is selected by folio_file_page(folio, idx). /Thomas > > Apart from those background questions looks good to me. > > Regards, > Christian. > > > + > > + folio_mark_accessed(to_folio); > > + folio_lock(to_folio); > > + folio_mark_dirty(to_folio); > > + copy_highpage(folio_file_page(to_folio, idx), page); > > + handle = ttm_backup_shmem_idx_to_handle(idx); > > + > > + if (writeback && !folio_mapped(to_folio) && > > + folio_clear_dirty_for_io(to_folio)) { > > + struct writeback_control wbc = { > > + .sync_mode = WB_SYNC_NONE, > > + .nr_to_write = SWAP_CLUSTER_MAX, > > + .range_start = 0, > > + .range_end = LLONG_MAX, > > + .for_reclaim = 1, > > + }; > > + folio_set_reclaim(to_folio); > > + ret = mapping->a_ops- > > >writepage(folio_file_page(to_folio, idx), &wbc); > > + if (!folio_test_writeback(to_folio)) > > + folio_clear_reclaim(to_folio); > > + /* If writepage succeeds, it unlocks the folio */ > > + if (ret) > > + folio_unlock(to_folio); > > + } else { > > + folio_unlock(to_folio); > > + } > > + > > + folio_put(to_folio); > > + > > + return handle; > > +} > > + > > +/** > > + * ttm_backup_fini() - Free the struct backup resources after last > > use. > > + * @backup: Pointer to the struct backup whose resources to free. > > + * > > + * After a call to this function, it's illegal to use the @backup > > pointer. > > + */ > > +void ttm_backup_fini(struct ttm_backup *backup) > > +{ > > + fput(ttm_backup_to_file(backup)); > > +} > > + > > +/** > > + * ttm_backup_bytes_avail() - Report the approximate number of > > bytes of backup space > > + * left for backup. > > + * > > + * This function is intended also for driver use to indicate > > whether a > > + * backup attempt is meaningful. > > + * > > + * Return: An approximate size of backup space available. > > + */ > > +u64 ttm_backup_bytes_avail(void) > > +{ > > + /* > > + * The idea behind backing up to shmem is that shmem > > objects may > > + * eventually be swapped out. So no point swapping out if > > there > > + * is no or low swap-space available. But the accuracy of > > this > > + * number also depends on shmem actually swapping out > > backed-up > > + * shmem objects without too much buffering. > > + */ > > + return (u64)get_nr_swap_pages() << PAGE_SHIFT; > > +} > > +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); > > + > > +/** > > + * ttm_backup_shmem_create() - Create a shmem-based struct backup. > > + * @size: The maximum size (in bytes) to back up. > > + * > > + * Create a backup utilizing shmem objects. > > + * > > + * Return: A pointer to a struct ttm_backup on success, > > + * an error pointer on error. > > + */ > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size) > > +{ > > + struct file *filp; > > + > > + filp = shmem_file_setup("ttm shmem backup", size, 0); > > + > > + return ttm_file_to_backup(filp); > > +} > > diff --git a/include/drm/ttm/ttm_backup.h > > b/include/drm/ttm/ttm_backup.h > > new file mode 100644 > > index 000000000000..20609da7e281 > > --- /dev/null > > +++ b/include/drm/ttm/ttm_backup.h > > @@ -0,0 +1,74 @@ > > +/* SPDX-License-Identifier: MIT */ > > +/* > > + * Copyright © 2024 Intel Corporation > > + */ > > + > > +#ifndef _TTM_BACKUP_H_ > > +#define _TTM_BACKUP_H_ > > + > > +#include <linux/mm_types.h> > > +#include <linux/shmem_fs.h> > > + > > +struct ttm_backup; > > + > > +/** > > + * ttm_backup_handle_to_page_ptr() - Convert handle to struct page > > pointer > > + * @handle: The handle to convert. > > + * > > + * Converts an opaque handle received from the > > + * struct ttm_backoup_ops::backup_page() function to an (invalid) > > + * struct page pointer suitable for a struct page array. > > + * > > + * Return: An (invalid) struct page pointer. > > + */ > > +static inline struct page * > > +ttm_backup_handle_to_page_ptr(unsigned long handle) > > +{ > > + return (struct page *)(handle << 1 | 1); > > +} > > + > > +/** > > + * ttm_backup_page_ptr_is_handle() - Whether a struct page pointer > > is a handle > > + * @page: The struct page pointer to check. > > + * > > + * Return: true if the struct page pointer is a handld returned > > from > > + * ttm_backup_handle_to_page_ptr(). False otherwise. > > + */ > > +static inline bool ttm_backup_page_ptr_is_handle(const struct page > > *page) > > +{ > > + return (unsigned long)page & 1; > > +} > > + > > +/** > > + * ttm_backup_page_ptr_to_handle() - Convert a struct page pointer > > to a handle > > + * @page: The struct page pointer to convert > > + * > > + * Return: The handle that was previously used in > > + * ttm_backup_handle_to_page_ptr() to obtain a struct page > > pointer, suitable > > + * for use as argument in the struct ttm_backup_ops drop() or > > + * copy_backed_up_page() functions. > > + */ > > +static inline unsigned long > > +ttm_backup_page_ptr_to_handle(const struct page *page) > > +{ > > + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); > > + return (unsigned long)page >> 1; > > +} > > + > > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle); > > + > > +int ttm_backup_copy_page(struct ttm_backup *backup, struct page > > *dst, > > + pgoff_t handle, bool intr); > > + > > +unsigned long > > +ttm_backup_backup_page(struct ttm_backup *backup, struct page > > *page, > > + bool writeback, pgoff_t idx, gfp_t > > page_gfp, > > + gfp_t alloc_gfp); > > + > > +void ttm_backup_fini(struct ttm_backup *backup); > > + > > +u64 ttm_backup_bytes_avail(void); > > + > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size); > > + > > +#endif >
Am 20.11.24 um 08:58 schrieb Thomas Hellström: > On Tue, 2024-11-19 at 14:40 +0100, Christian König wrote: >> [SNIP] >>> + >>> +/* >>> + * Casting from randomized struct file * to struct ttm_backup * is >>> fine since >>> + * struct ttm_backup is never defined nor dereferenced. >>> + */ >>> +static struct file *ttm_backup_to_file(struct ttm_backup *backup) >> Do I get it right that struct ttm_backup is never really defined? > Yes. > >> What >> purpose does that have? > It's to make the struct ttm_backup opaque to the users of the > ttm_backup interface, so that the implementation doesn't have to worry > about the user making illegal assumptions about the implementation. That is usually done with a typedef and one of the few cases where typedefs are actually advised to be used. [SNIP] >>> + * >>> + * Context: If called from reclaim context, the caller needs to >>> + * assert that the shrinker gfp has __GFP_FS set, to avoid >>> + * deadlocking on lock_page(). If @writeback is set to true and >>> + * called from reclaim context, the caller also needs to assert >>> + * that the shrinker gfp has __GFP_IO set, since without it, >>> + * we're not allowed to start backup IO. >>> + * >>> + * Return: A handle on success. 0 on failure. >>> + * (This is following the swp_entry_t convention). >>> + * >>> + * Note: This function could be extended to back up a folio and >>> + * implementations would then split the folio internally if >>> needed. >>> + * Drawback is that the caller would then have to keep track of >>> + * the folio size- and usage. >>> + */ >>> +unsigned long >>> +ttm_backup_backup_page(struct ttm_backup *backup, struct page >>> *page, >>> + bool writeback, pgoff_t idx, gfp_t >>> page_gfp, >>> + gfp_t alloc_gfp) >>> +{ >>> + struct file *filp = ttm_backup_to_file(backup); >>> + struct address_space *mapping = filp->f_mapping; >>> + unsigned long handle = 0; >>> + struct folio *to_folio; >>> + int ret; >>> + >>> + to_folio = shmem_read_folio_gfp(mapping, idx, alloc_gfp); >>> + if (IS_ERR(to_folio)) >>> + return handle; Probably better to explicitly return 0 here. And BTW why are we using 0 as indication for an error? Couldn't we just use a long as return value and return a proper -errno here? >> Just that I sleep better: This can never return a folio larger than a >> page, doesn't it? > The interface definitely allows for returning larger folios, but the > individual page in the folio is selected by folio_file_page(folio, > idx). Ah, yeah completely missed that and was really wondering why that would work. > > /Thomas > > >> Apart from those background questions looks good to me. >> >> Regards, >> Christian. >> >>> + >>> + folio_mark_accessed(to_folio); >>> + folio_lock(to_folio); >>> + folio_mark_dirty(to_folio); >>> + copy_highpage(folio_file_page(to_folio, idx), page); >>> + handle = ttm_backup_shmem_idx_to_handle(idx); >>> + >>> + if (writeback && !folio_mapped(to_folio) && >>> + folio_clear_dirty_for_io(to_folio)) { >>> + struct writeback_control wbc = { >>> + .sync_mode = WB_SYNC_NONE, >>> + .nr_to_write = SWAP_CLUSTER_MAX, >>> + .range_start = 0, >>> + .range_end = LLONG_MAX, >>> + .for_reclaim = 1, >>> + }; >>> + folio_set_reclaim(to_folio); >>> + ret = mapping->a_ops- >>>> writepage(folio_file_page(to_folio, idx), &wbc); >>> + if (!folio_test_writeback(to_folio)) >>> + folio_clear_reclaim(to_folio); >>> + /* If writepage succeeds, it unlocks the folio */ >>> + if (ret) >>> + folio_unlock(to_folio); The code ignores the error and potentially deserves an explanation for that. Regards, Christian. >>> + } else { >>> + folio_unlock(to_folio); >>> + } >>> + >>> + folio_put(to_folio); >>> + >>> + return handle; >>> +} >>> + >>> +/** >>> + * ttm_backup_fini() - Free the struct backup resources after last >>> use. >>> + * @backup: Pointer to the struct backup whose resources to free. >>> + * >>> + * After a call to this function, it's illegal to use the @backup >>> pointer. >>> + */ >>> +void ttm_backup_fini(struct ttm_backup *backup) >>> +{ >>> + fput(ttm_backup_to_file(backup)); >>> +} >>> + >>> +/** >>> + * ttm_backup_bytes_avail() - Report the approximate number of >>> bytes of backup space >>> + * left for backup. >>> + * >>> + * This function is intended also for driver use to indicate >>> whether a >>> + * backup attempt is meaningful. >>> + * >>> + * Return: An approximate size of backup space available. >>> + */ >>> +u64 ttm_backup_bytes_avail(void) >>> +{ >>> + /* >>> + * The idea behind backing up to shmem is that shmem >>> objects may >>> + * eventually be swapped out. So no point swapping out if >>> there >>> + * is no or low swap-space available. But the accuracy of >>> this >>> + * number also depends on shmem actually swapping out >>> backed-up >>> + * shmem objects without too much buffering. >>> + */ >>> + return (u64)get_nr_swap_pages() << PAGE_SHIFT; >>> +} >>> +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); >>> + >>> +/** >>> + * ttm_backup_shmem_create() - Create a shmem-based struct backup. >>> + * @size: The maximum size (in bytes) to back up. >>> + * >>> + * Create a backup utilizing shmem objects. >>> + * >>> + * Return: A pointer to a struct ttm_backup on success, >>> + * an error pointer on error. >>> + */ >>> +struct ttm_backup *ttm_backup_shmem_create(loff_t size) >>> +{ >>> + struct file *filp; >>> + >>> + filp = shmem_file_setup("ttm shmem backup", size, 0); >>> + >>> + return ttm_file_to_backup(filp); >>> +} >>> diff --git a/include/drm/ttm/ttm_backup.h >>> b/include/drm/ttm/ttm_backup.h >>> new file mode 100644 >>> index 000000000000..20609da7e281 >>> --- /dev/null >>> +++ b/include/drm/ttm/ttm_backup.h >>> @@ -0,0 +1,74 @@ >>> +/* SPDX-License-Identifier: MIT */ >>> +/* >>> + * Copyright © 2024 Intel Corporation >>> + */ >>> + >>> +#ifndef _TTM_BACKUP_H_ >>> +#define _TTM_BACKUP_H_ >>> + >>> +#include <linux/mm_types.h> >>> +#include <linux/shmem_fs.h> >>> + >>> +struct ttm_backup; >>> + >>> +/** >>> + * ttm_backup_handle_to_page_ptr() - Convert handle to struct page >>> pointer >>> + * @handle: The handle to convert. >>> + * >>> + * Converts an opaque handle received from the >>> + * struct ttm_backoup_ops::backup_page() function to an (invalid) >>> + * struct page pointer suitable for a struct page array. >>> + * >>> + * Return: An (invalid) struct page pointer. >>> + */ >>> +static inline struct page * >>> +ttm_backup_handle_to_page_ptr(unsigned long handle) >>> +{ >>> + return (struct page *)(handle << 1 | 1); >>> +} >>> + >>> +/** >>> + * ttm_backup_page_ptr_is_handle() - Whether a struct page pointer >>> is a handle >>> + * @page: The struct page pointer to check. >>> + * >>> + * Return: true if the struct page pointer is a handld returned >>> from >>> + * ttm_backup_handle_to_page_ptr(). False otherwise. >>> + */ >>> +static inline bool ttm_backup_page_ptr_is_handle(const struct page >>> *page) >>> +{ >>> + return (unsigned long)page & 1; >>> +} >>> + >>> +/** >>> + * ttm_backup_page_ptr_to_handle() - Convert a struct page pointer >>> to a handle >>> + * @page: The struct page pointer to convert >>> + * >>> + * Return: The handle that was previously used in >>> + * ttm_backup_handle_to_page_ptr() to obtain a struct page >>> pointer, suitable >>> + * for use as argument in the struct ttm_backup_ops drop() or >>> + * copy_backed_up_page() functions. >>> + */ >>> +static inline unsigned long >>> +ttm_backup_page_ptr_to_handle(const struct page *page) >>> +{ >>> + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); >>> + return (unsigned long)page >> 1; >>> +} >>> + >>> +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle); >>> + >>> +int ttm_backup_copy_page(struct ttm_backup *backup, struct page >>> *dst, >>> + pgoff_t handle, bool intr); >>> + >>> +unsigned long >>> +ttm_backup_backup_page(struct ttm_backup *backup, struct page >>> *page, >>> + bool writeback, pgoff_t idx, gfp_t >>> page_gfp, >>> + gfp_t alloc_gfp); >>> + >>> +void ttm_backup_fini(struct ttm_backup *backup); >>> + >>> +u64 ttm_backup_bytes_avail(void); >>> + >>> +struct ttm_backup *ttm_backup_shmem_create(loff_t size); >>> + >>> +#endif
On Wed, 2024-11-20 at 10:24 +0100, Christian König wrote: > Am 20.11.24 um 08:58 schrieb Thomas Hellström: > > On Tue, 2024-11-19 at 14:40 +0100, Christian König wrote: > > > [SNIP] > > > > + > > > > +/* > > > > + * Casting from randomized struct file * to struct ttm_backup > > > > * is > > > > fine since > > > > + * struct ttm_backup is never defined nor dereferenced. > > > > + */ > > > > +static struct file *ttm_backup_to_file(struct ttm_backup > > > > *backup) > > > Do I get it right that struct ttm_backup is never really defined? > > Yes. > > > > > What > > > purpose does that have? > > It's to make the struct ttm_backup opaque to the users of the > > ttm_backup interface, so that the implementation doesn't have to > > worry > > about the user making illegal assumptions about the implementation. > > That is usually done with a typedef and one of the few cases where > typedefs are actually advised to be used. > Well wouldn't ttm_backup.h then have to include the declaration of struct file plus a typedef that would probably raise many eyebrows even if it's ok to use it there? Having the header just declare a struct without providing a definition is the typical way of hiding the implementation and avoid includes, no? If you insist we can drop the struct ttm_backup * and just use struct file, but then again if we change the implementation to allow for backuping to a file or similar that needs to be re-done, so as said unless you insist I'd rather keep it as is. > > [SNIP] > > > > + * > > > > + * Context: If called from reclaim context, the caller needs > > > > to > > > > + * assert that the shrinker gfp has __GFP_FS set, to avoid > > > > + * deadlocking on lock_page(). If @writeback is set to true > > > > and > > > > + * called from reclaim context, the caller also needs to > > > > assert > > > > + * that the shrinker gfp has __GFP_IO set, since without it, > > > > + * we're not allowed to start backup IO. > > > > + * > > > > + * Return: A handle on success. 0 on failure. > > > > + * (This is following the swp_entry_t convention). > > > > + * > > > > + * Note: This function could be extended to back up a folio > > > > and > > > > + * implementations would then split the folio internally if > > > > needed. > > > > + * Drawback is that the caller would then have to keep track > > > > of > > > > + * the folio size- and usage. > > > > + */ > > > > +unsigned long > > > > +ttm_backup_backup_page(struct ttm_backup *backup, struct page > > > > *page, > > > > + bool writeback, pgoff_t idx, gfp_t > > > > page_gfp, > > > > + gfp_t alloc_gfp) > > > > +{ > > > > + struct file *filp = ttm_backup_to_file(backup); > > > > + struct address_space *mapping = filp->f_mapping; > > > > + unsigned long handle = 0; > > > > + struct folio *to_folio; > > > > + int ret; > > > > + > > > > + to_folio = shmem_read_folio_gfp(mapping, idx, > > > > alloc_gfp); > > > > + if (IS_ERR(to_folio)) > > > > + return handle; > > Probably better to explicitly return 0 here. OK, > > And BTW why are we using 0 as indication for an error? Couldn't we > just > use a long as return value and return a proper -errno here? 0 is the swp_entry_t error value which is the convention also used for the handles, so rather than inventing something new It'd be good to keep to something that would work even with handles aliased to swp_entry_t if we'd need to resort to that at some point. > > > > Just that I sleep better: This can never return a folio larger > > > than a > > > page, doesn't it? > > The interface definitely allows for returning larger folios, but > > the > > individual page in the folio is selected by folio_file_page(folio, > > idx). > > Ah, yeah completely missed that and was really wondering why that > would > work. Thanks, Thomas > > > > > /Thomas > > > > > > > Apart from those background questions looks good to me. > > > > > > Regards, > > > Christian. > > > > > > > + > > > > + folio_mark_accessed(to_folio); > > > > + folio_lock(to_folio); > > > > + folio_mark_dirty(to_folio); > > > > + copy_highpage(folio_file_page(to_folio, idx), page); > > > > + handle = ttm_backup_shmem_idx_to_handle(idx); > > > > + > > > > + if (writeback && !folio_mapped(to_folio) && > > > > + folio_clear_dirty_for_io(to_folio)) { > > > > + struct writeback_control wbc = { > > > > + .sync_mode = WB_SYNC_NONE, > > > > + .nr_to_write = SWAP_CLUSTER_MAX, > > > > + .range_start = 0, > > > > + .range_end = LLONG_MAX, > > > > + .for_reclaim = 1, > > > > + }; > > > > + folio_set_reclaim(to_folio); > > > > + ret = mapping->a_ops- > > > > > writepage(folio_file_page(to_folio, idx), &wbc); > > > > + if (!folio_test_writeback(to_folio)) > > > > + folio_clear_reclaim(to_folio); > > > > + /* If writepage succeeds, it unlocks the folio > > > > */ > > > > + if (ret) > > > > + folio_unlock(to_folio); > > The code ignores the error and potentially deserves an explanation > for that. > > Regards, > Christian. > > > > > + } else { > > > > + folio_unlock(to_folio); > > > > + } > > > > + > > > > + folio_put(to_folio); > > > > + > > > > + return handle; > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_fini() - Free the struct backup resources after > > > > last > > > > use. > > > > + * @backup: Pointer to the struct backup whose resources to > > > > free. > > > > + * > > > > + * After a call to this function, it's illegal to use the > > > > @backup > > > > pointer. > > > > + */ > > > > +void ttm_backup_fini(struct ttm_backup *backup) > > > > +{ > > > > + fput(ttm_backup_to_file(backup)); > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_bytes_avail() - Report the approximate number of > > > > bytes of backup space > > > > + * left for backup. > > > > + * > > > > + * This function is intended also for driver use to indicate > > > > whether a > > > > + * backup attempt is meaningful. > > > > + * > > > > + * Return: An approximate size of backup space available. > > > > + */ > > > > +u64 ttm_backup_bytes_avail(void) > > > > +{ > > > > + /* > > > > + * The idea behind backing up to shmem is that shmem > > > > objects may > > > > + * eventually be swapped out. So no point swapping out > > > > if > > > > there > > > > + * is no or low swap-space available. But the accuracy > > > > of > > > > this > > > > + * number also depends on shmem actually swapping out > > > > backed-up > > > > + * shmem objects without too much buffering. > > > > + */ > > > > + return (u64)get_nr_swap_pages() << PAGE_SHIFT; > > > > +} > > > > +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); > > > > + > > > > +/** > > > > + * ttm_backup_shmem_create() - Create a shmem-based struct > > > > backup. > > > > + * @size: The maximum size (in bytes) to back up. > > > > + * > > > > + * Create a backup utilizing shmem objects. > > > > + * > > > > + * Return: A pointer to a struct ttm_backup on success, > > > > + * an error pointer on error. > > > > + */ > > > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size) > > > > +{ > > > > + struct file *filp; > > > > + > > > > + filp = shmem_file_setup("ttm shmem backup", size, 0); > > > > + > > > > + return ttm_file_to_backup(filp); > > > > +} > > > > diff --git a/include/drm/ttm/ttm_backup.h > > > > b/include/drm/ttm/ttm_backup.h > > > > new file mode 100644 > > > > index 000000000000..20609da7e281 > > > > --- /dev/null > > > > +++ b/include/drm/ttm/ttm_backup.h > > > > @@ -0,0 +1,74 @@ > > > > +/* SPDX-License-Identifier: MIT */ > > > > +/* > > > > + * Copyright © 2024 Intel Corporation > > > > + */ > > > > + > > > > +#ifndef _TTM_BACKUP_H_ > > > > +#define _TTM_BACKUP_H_ > > > > + > > > > +#include <linux/mm_types.h> > > > > +#include <linux/shmem_fs.h> > > > > + > > > > +struct ttm_backup; > > > > + > > > > +/** > > > > + * ttm_backup_handle_to_page_ptr() - Convert handle to struct > > > > page > > > > pointer > > > > + * @handle: The handle to convert. > > > > + * > > > > + * Converts an opaque handle received from the > > > > + * struct ttm_backoup_ops::backup_page() function to an > > > > (invalid) > > > > + * struct page pointer suitable for a struct page array. > > > > + * > > > > + * Return: An (invalid) struct page pointer. > > > > + */ > > > > +static inline struct page * > > > > +ttm_backup_handle_to_page_ptr(unsigned long handle) > > > > +{ > > > > + return (struct page *)(handle << 1 | 1); > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_page_ptr_is_handle() - Whether a struct page > > > > pointer > > > > is a handle > > > > + * @page: The struct page pointer to check. > > > > + * > > > > + * Return: true if the struct page pointer is a handld > > > > returned > > > > from > > > > + * ttm_backup_handle_to_page_ptr(). False otherwise. > > > > + */ > > > > +static inline bool ttm_backup_page_ptr_is_handle(const struct > > > > page > > > > *page) > > > > +{ > > > > + return (unsigned long)page & 1; > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_page_ptr_to_handle() - Convert a struct page > > > > pointer > > > > to a handle > > > > + * @page: The struct page pointer to convert > > > > + * > > > > + * Return: The handle that was previously used in > > > > + * ttm_backup_handle_to_page_ptr() to obtain a struct page > > > > pointer, suitable > > > > + * for use as argument in the struct ttm_backup_ops drop() or > > > > + * copy_backed_up_page() functions. > > > > + */ > > > > +static inline unsigned long > > > > +ttm_backup_page_ptr_to_handle(const struct page *page) > > > > +{ > > > > + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); > > > > + return (unsigned long)page >> 1; > > > > +} > > > > + > > > > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t > > > > handle); > > > > + > > > > +int ttm_backup_copy_page(struct ttm_backup *backup, struct > > > > page > > > > *dst, > > > > + pgoff_t handle, bool intr); > > > > + > > > > +unsigned long > > > > +ttm_backup_backup_page(struct ttm_backup *backup, struct page > > > > *page, > > > > + bool writeback, pgoff_t idx, gfp_t > > > > page_gfp, > > > > + gfp_t alloc_gfp); > > > > + > > > > +void ttm_backup_fini(struct ttm_backup *backup); > > > > + > > > > +u64 ttm_backup_bytes_avail(void); > > > > + > > > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size); > > > > + > > > > +#endif
Am 20.11.24 um 11:34 schrieb Thomas Hellström: > On Wed, 2024-11-20 at 10:24 +0100, Christian König wrote: >> Am 20.11.24 um 08:58 schrieb Thomas Hellström: >>> On Tue, 2024-11-19 at 14:40 +0100, Christian König wrote: >>>> [SNIP] >>>>> + >>>>> +/* >>>>> + * Casting from randomized struct file * to struct ttm_backup >>>>> * is >>>>> fine since >>>>> + * struct ttm_backup is never defined nor dereferenced. >>>>> + */ >>>>> +static struct file *ttm_backup_to_file(struct ttm_backup >>>>> *backup) >>>> Do I get it right that struct ttm_backup is never really defined? >>> Yes. >>> >>>> What >>>> purpose does that have? >>> It's to make the struct ttm_backup opaque to the users of the >>> ttm_backup interface, so that the implementation doesn't have to >>> worry >>> about the user making illegal assumptions about the implementation. >> That is usually done with a typedef and one of the few cases where >> typedefs are actually advised to be used. >> > Well wouldn't ttm_backup.h then have to include the declaration of > struct file plus a typedef that would probably raise many eyebrows even > if it's ok to use it there? No, what you do is something like this: typedef struct ttm_backup *ttm_backup; Then struct ttm_backup is either never defined or only inside your C file but not the header. > Having the header just declare a struct without providing a definition > is the typical way of hiding the implementation and avoid includes, no? > > If you insist we can drop the struct ttm_backup * and just use struct > file, but then again if we change the implementation to allow for > backuping to a file or similar that needs to be re-done, so as said > unless you insist I'd rather keep it as is. Abstracting that is ok, I was just wondering about why you do it like this. > >> [SNIP] >>>>> + * >>>>> + * Context: If called from reclaim context, the caller needs >>>>> to >>>>> + * assert that the shrinker gfp has __GFP_FS set, to avoid >>>>> + * deadlocking on lock_page(). If @writeback is set to true >>>>> and >>>>> + * called from reclaim context, the caller also needs to >>>>> assert >>>>> + * that the shrinker gfp has __GFP_IO set, since without it, >>>>> + * we're not allowed to start backup IO. >>>>> + * >>>>> + * Return: A handle on success. 0 on failure. >>>>> + * (This is following the swp_entry_t convention). >>>>> + * >>>>> + * Note: This function could be extended to back up a folio >>>>> and >>>>> + * implementations would then split the folio internally if >>>>> needed. >>>>> + * Drawback is that the caller would then have to keep track >>>>> of >>>>> + * the folio size- and usage. >>>>> + */ >>>>> +unsigned long >>>>> +ttm_backup_backup_page(struct ttm_backup *backup, struct page >>>>> *page, >>>>> + bool writeback, pgoff_t idx, gfp_t >>>>> page_gfp, >>>>> + gfp_t alloc_gfp) >>>>> +{ >>>>> + struct file *filp = ttm_backup_to_file(backup); >>>>> + struct address_space *mapping = filp->f_mapping; >>>>> + unsigned long handle = 0; >>>>> + struct folio *to_folio; >>>>> + int ret; >>>>> + >>>>> + to_folio = shmem_read_folio_gfp(mapping, idx, >>>>> alloc_gfp); >>>>> + if (IS_ERR(to_folio)) >>>>> + return handle; >> Probably better to explicitly return 0 here. > OK, > >> And BTW why are we using 0 as indication for an error? Couldn't we >> just >> use a long as return value and return a proper -errno here? > 0 is the swp_entry_t error value which is the convention also used for > the handles, so rather than inventing something new It'd be good to > keep to something that would work even with handles aliased to > swp_entry_t if we'd need to resort to that at some point. Uff, yeah but that is an implementation detail of the swap subsystem caused by how we store the swapped out entries inside CPU PTEs. I would strongly try to avoid that here. Was already wondering why we use long as return value and s64. Regards, Christian. > >>>> Just that I sleep better: This can never return a folio larger >>>> than a >>>> page, doesn't it? >>> The interface definitely allows for returning larger folios, but >>> the >>> individual page in the folio is selected by folio_file_page(folio, >>> idx). >> Ah, yeah completely missed that and was really wondering why that >> would >> work. > Thanks, > Thomas > >>> /Thomas >>> >>> >>>> Apart from those background questions looks good to me. >>>> >>>> Regards, >>>> Christian. >>>> >>>>> + >>>>> + folio_mark_accessed(to_folio); >>>>> + folio_lock(to_folio); >>>>> + folio_mark_dirty(to_folio); >>>>> + copy_highpage(folio_file_page(to_folio, idx), page); >>>>> + handle = ttm_backup_shmem_idx_to_handle(idx); >>>>> + >>>>> + if (writeback && !folio_mapped(to_folio) && >>>>> + folio_clear_dirty_for_io(to_folio)) { >>>>> + struct writeback_control wbc = { >>>>> + .sync_mode = WB_SYNC_NONE, >>>>> + .nr_to_write = SWAP_CLUSTER_MAX, >>>>> + .range_start = 0, >>>>> + .range_end = LLONG_MAX, >>>>> + .for_reclaim = 1, >>>>> + }; >>>>> + folio_set_reclaim(to_folio); >>>>> + ret = mapping->a_ops- >>>>>> writepage(folio_file_page(to_folio, idx), &wbc); >>>>> + if (!folio_test_writeback(to_folio)) >>>>> + folio_clear_reclaim(to_folio); >>>>> + /* If writepage succeeds, it unlocks the folio >>>>> */ >>>>> + if (ret) >>>>> + folio_unlock(to_folio); >> The code ignores the error and potentially deserves an explanation >> for that. >> >> Regards, >> Christian. >> >>>>> + } else { >>>>> + folio_unlock(to_folio); >>>>> + } >>>>> + >>>>> + folio_put(to_folio); >>>>> + >>>>> + return handle; >>>>> +} >>>>> + >>>>> +/** >>>>> + * ttm_backup_fini() - Free the struct backup resources after >>>>> last >>>>> use. >>>>> + * @backup: Pointer to the struct backup whose resources to >>>>> free. >>>>> + * >>>>> + * After a call to this function, it's illegal to use the >>>>> @backup >>>>> pointer. >>>>> + */ >>>>> +void ttm_backup_fini(struct ttm_backup *backup) >>>>> +{ >>>>> + fput(ttm_backup_to_file(backup)); >>>>> +} >>>>> + >>>>> +/** >>>>> + * ttm_backup_bytes_avail() - Report the approximate number of >>>>> bytes of backup space >>>>> + * left for backup. >>>>> + * >>>>> + * This function is intended also for driver use to indicate >>>>> whether a >>>>> + * backup attempt is meaningful. >>>>> + * >>>>> + * Return: An approximate size of backup space available. >>>>> + */ >>>>> +u64 ttm_backup_bytes_avail(void) >>>>> +{ >>>>> + /* >>>>> + * The idea behind backing up to shmem is that shmem >>>>> objects may >>>>> + * eventually be swapped out. So no point swapping out >>>>> if >>>>> there >>>>> + * is no or low swap-space available. But the accuracy >>>>> of >>>>> this >>>>> + * number also depends on shmem actually swapping out >>>>> backed-up >>>>> + * shmem objects without too much buffering. >>>>> + */ >>>>> + return (u64)get_nr_swap_pages() << PAGE_SHIFT; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); >>>>> + >>>>> +/** >>>>> + * ttm_backup_shmem_create() - Create a shmem-based struct >>>>> backup. >>>>> + * @size: The maximum size (in bytes) to back up. >>>>> + * >>>>> + * Create a backup utilizing shmem objects. >>>>> + * >>>>> + * Return: A pointer to a struct ttm_backup on success, >>>>> + * an error pointer on error. >>>>> + */ >>>>> +struct ttm_backup *ttm_backup_shmem_create(loff_t size) >>>>> +{ >>>>> + struct file *filp; >>>>> + >>>>> + filp = shmem_file_setup("ttm shmem backup", size, 0); >>>>> + >>>>> + return ttm_file_to_backup(filp); >>>>> +} >>>>> diff --git a/include/drm/ttm/ttm_backup.h >>>>> b/include/drm/ttm/ttm_backup.h >>>>> new file mode 100644 >>>>> index 000000000000..20609da7e281 >>>>> --- /dev/null >>>>> +++ b/include/drm/ttm/ttm_backup.h >>>>> @@ -0,0 +1,74 @@ >>>>> +/* SPDX-License-Identifier: MIT */ >>>>> +/* >>>>> + * Copyright © 2024 Intel Corporation >>>>> + */ >>>>> + >>>>> +#ifndef _TTM_BACKUP_H_ >>>>> +#define _TTM_BACKUP_H_ >>>>> + >>>>> +#include <linux/mm_types.h> >>>>> +#include <linux/shmem_fs.h> >>>>> + >>>>> +struct ttm_backup; >>>>> + >>>>> +/** >>>>> + * ttm_backup_handle_to_page_ptr() - Convert handle to struct >>>>> page >>>>> pointer >>>>> + * @handle: The handle to convert. >>>>> + * >>>>> + * Converts an opaque handle received from the >>>>> + * struct ttm_backoup_ops::backup_page() function to an >>>>> (invalid) >>>>> + * struct page pointer suitable for a struct page array. >>>>> + * >>>>> + * Return: An (invalid) struct page pointer. >>>>> + */ >>>>> +static inline struct page * >>>>> +ttm_backup_handle_to_page_ptr(unsigned long handle) >>>>> +{ >>>>> + return (struct page *)(handle << 1 | 1); >>>>> +} >>>>> + >>>>> +/** >>>>> + * ttm_backup_page_ptr_is_handle() - Whether a struct page >>>>> pointer >>>>> is a handle >>>>> + * @page: The struct page pointer to check. >>>>> + * >>>>> + * Return: true if the struct page pointer is a handld >>>>> returned >>>>> from >>>>> + * ttm_backup_handle_to_page_ptr(). False otherwise. >>>>> + */ >>>>> +static inline bool ttm_backup_page_ptr_is_handle(const struct >>>>> page >>>>> *page) >>>>> +{ >>>>> + return (unsigned long)page & 1; >>>>> +} >>>>> + >>>>> +/** >>>>> + * ttm_backup_page_ptr_to_handle() - Convert a struct page >>>>> pointer >>>>> to a handle >>>>> + * @page: The struct page pointer to convert >>>>> + * >>>>> + * Return: The handle that was previously used in >>>>> + * ttm_backup_handle_to_page_ptr() to obtain a struct page >>>>> pointer, suitable >>>>> + * for use as argument in the struct ttm_backup_ops drop() or >>>>> + * copy_backed_up_page() functions. >>>>> + */ >>>>> +static inline unsigned long >>>>> +ttm_backup_page_ptr_to_handle(const struct page *page) >>>>> +{ >>>>> + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); >>>>> + return (unsigned long)page >> 1; >>>>> +} >>>>> + >>>>> +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t >>>>> handle); >>>>> + >>>>> +int ttm_backup_copy_page(struct ttm_backup *backup, struct >>>>> page >>>>> *dst, >>>>> + pgoff_t handle, bool intr); >>>>> + >>>>> +unsigned long >>>>> +ttm_backup_backup_page(struct ttm_backup *backup, struct page >>>>> *page, >>>>> + bool writeback, pgoff_t idx, gfp_t >>>>> page_gfp, >>>>> + gfp_t alloc_gfp); >>>>> + >>>>> +void ttm_backup_fini(struct ttm_backup *backup); >>>>> + >>>>> +u64 ttm_backup_bytes_avail(void); >>>>> + >>>>> +struct ttm_backup *ttm_backup_shmem_create(loff_t size); >>>>> + >>>>> +#endif
On Wed, 2024-11-20 at 11:50 +0100, Christian König wrote: > Am 20.11.24 um 11:34 schrieb Thomas Hellström: > > On Wed, 2024-11-20 at 10:24 +0100, Christian König wrote: > > > Am 20.11.24 um 08:58 schrieb Thomas Hellström: > > > > On Tue, 2024-11-19 at 14:40 +0100, Christian König wrote: > > > > > [SNIP] > > > > > > + > > > > > > +/* > > > > > > + * Casting from randomized struct file * to struct > > > > > > ttm_backup > > > > > > * is > > > > > > fine since > > > > > > + * struct ttm_backup is never defined nor dereferenced. > > > > > > + */ > > > > > > +static struct file *ttm_backup_to_file(struct ttm_backup > > > > > > *backup) > > > > > Do I get it right that struct ttm_backup is never really > > > > > defined? > > > > Yes. > > > > > > > > > What > > > > > purpose does that have? > > > > It's to make the struct ttm_backup opaque to the users of the > > > > ttm_backup interface, so that the implementation doesn't have > > > > to > > > > worry > > > > about the user making illegal assumptions about the > > > > implementation. > > > That is usually done with a typedef and one of the few cases > > > where > > > typedefs are actually advised to be used. > > > > > Well wouldn't ttm_backup.h then have to include the declaration of > > struct file plus a typedef that would probably raise many eyebrows > > even > > if it's ok to use it there? > > No, what you do is something like this: > > typedef struct ttm_backup *ttm_backup; > > Then struct ttm_backup is either never defined or only inside your C > file but not the header. > > > Having the header just declare a struct without providing a > > definition > > is the typical way of hiding the implementation and avoid includes, > > no? > > > > If you insist we can drop the struct ttm_backup * and just use > > struct > > file, but then again if we change the implementation to allow for > > backuping to a file or similar that needs to be re-done, so as said > > unless you insist I'd rather keep it as is. > > Abstracting that is ok, I was just wondering about why you do it like > this. > > > > > > [SNIP] > > > > > > + * > > > > > > + * Context: If called from reclaim context, the caller > > > > > > needs > > > > > > to > > > > > > + * assert that the shrinker gfp has __GFP_FS set, to avoid > > > > > > + * deadlocking on lock_page(). If @writeback is set to > > > > > > true > > > > > > and > > > > > > + * called from reclaim context, the caller also needs to > > > > > > assert > > > > > > + * that the shrinker gfp has __GFP_IO set, since without > > > > > > it, > > > > > > + * we're not allowed to start backup IO. > > > > > > + * > > > > > > + * Return: A handle on success. 0 on failure. > > > > > > + * (This is following the swp_entry_t convention). > > > > > > + * > > > > > > + * Note: This function could be extended to back up a > > > > > > folio > > > > > > and > > > > > > + * implementations would then split the folio internally > > > > > > if > > > > > > needed. > > > > > > + * Drawback is that the caller would then have to keep > > > > > > track > > > > > > of > > > > > > + * the folio size- and usage. > > > > > > + */ > > > > > > +unsigned long > > > > > > +ttm_backup_backup_page(struct ttm_backup *backup, struct > > > > > > page > > > > > > *page, > > > > > > + bool writeback, pgoff_t idx, gfp_t > > > > > > page_gfp, > > > > > > + gfp_t alloc_gfp) > > > > > > +{ > > > > > > + struct file *filp = ttm_backup_to_file(backup); > > > > > > + struct address_space *mapping = filp->f_mapping; > > > > > > + unsigned long handle = 0; > > > > > > + struct folio *to_folio; > > > > > > + int ret; > > > > > > + > > > > > > + to_folio = shmem_read_folio_gfp(mapping, idx, > > > > > > alloc_gfp); > > > > > > + if (IS_ERR(to_folio)) > > > > > > + return handle; > > > Probably better to explicitly return 0 here. > > OK, > > > > > And BTW why are we using 0 as indication for an error? Couldn't > > > we > > > just > > > use a long as return value and return a proper -errno here? > > 0 is the swp_entry_t error value which is the convention also used > > for > > the handles, so rather than inventing something new It'd be good to > > keep to something that would work even with handles aliased to > > swp_entry_t if we'd need to resort to that at some point. > > Uff, yeah but that is an implementation detail of the swap subsystem > caused by how we store the swapped out entries inside CPU PTEs. > > I would strongly try to avoid that here. Was already wondering why we > use long as return value and s64. That is true, The background here is that the initial implementation allowed for direct insertion into the swap cache, and then the handles returned would be (unsigned long)swp_entry_t, and the interface was kept to allow for such a change should it be necessary. But yeah I guess a logical consequence of removing support for alternative backup backends would be to drop explicit support for that. So I can change that to s64 np. /Thomas > > Regards, > Christian. > > > > > > > > Just that I sleep better: This can never return a folio > > > > > larger > > > > > than a > > > > > page, doesn't it? > > > > The interface definitely allows for returning larger folios, > > > > but > > > > the > > > > individual page in the folio is selected by > > > > folio_file_page(folio, > > > > idx). > > > Ah, yeah completely missed that and was really wondering why that > > > would > > > work. > > Thanks, > > Thomas > > > > > > /Thomas > > > > > > > > > > > > > Apart from those background questions looks good to me. > > > > > > > > > > Regards, > > > > > Christian. > > > > > > > > > > > + > > > > > > + folio_mark_accessed(to_folio); > > > > > > + folio_lock(to_folio); > > > > > > + folio_mark_dirty(to_folio); > > > > > > + copy_highpage(folio_file_page(to_folio, idx), > > > > > > page); > > > > > > + handle = ttm_backup_shmem_idx_to_handle(idx); > > > > > > + > > > > > > + if (writeback && !folio_mapped(to_folio) && > > > > > > + folio_clear_dirty_for_io(to_folio)) { > > > > > > + struct writeback_control wbc = { > > > > > > + .sync_mode = WB_SYNC_NONE, > > > > > > + .nr_to_write = SWAP_CLUSTER_MAX, > > > > > > + .range_start = 0, > > > > > > + .range_end = LLONG_MAX, > > > > > > + .for_reclaim = 1, > > > > > > + }; > > > > > > + folio_set_reclaim(to_folio); > > > > > > + ret = mapping->a_ops- > > > > > > > writepage(folio_file_page(to_folio, idx), &wbc); > > > > > > + if (!folio_test_writeback(to_folio)) > > > > > > + folio_clear_reclaim(to_folio); > > > > > > + /* If writepage succeeds, it unlocks the > > > > > > folio > > > > > > */ > > > > > > + if (ret) > > > > > > + folio_unlock(to_folio); > > > The code ignores the error and potentially deserves an > > > explanation > > > for that. > > > > > > Regards, > > > Christian. > > > > > > > > > + } else { > > > > > > + folio_unlock(to_folio); > > > > > > + } > > > > > > + > > > > > > + folio_put(to_folio); > > > > > > + > > > > > > + return handle; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * ttm_backup_fini() - Free the struct backup resources > > > > > > after > > > > > > last > > > > > > use. > > > > > > + * @backup: Pointer to the struct backup whose resources > > > > > > to > > > > > > free. > > > > > > + * > > > > > > + * After a call to this function, it's illegal to use the > > > > > > @backup > > > > > > pointer. > > > > > > + */ > > > > > > +void ttm_backup_fini(struct ttm_backup *backup) > > > > > > +{ > > > > > > + fput(ttm_backup_to_file(backup)); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * ttm_backup_bytes_avail() - Report the approximate > > > > > > number of > > > > > > bytes of backup space > > > > > > + * left for backup. > > > > > > + * > > > > > > + * This function is intended also for driver use to > > > > > > indicate > > > > > > whether a > > > > > > + * backup attempt is meaningful. > > > > > > + * > > > > > > + * Return: An approximate size of backup space available. > > > > > > + */ > > > > > > +u64 ttm_backup_bytes_avail(void) > > > > > > +{ > > > > > > + /* > > > > > > + * The idea behind backing up to shmem is that > > > > > > shmem > > > > > > objects may > > > > > > + * eventually be swapped out. So no point swapping > > > > > > out > > > > > > if > > > > > > there > > > > > > + * is no or low swap-space available. But the > > > > > > accuracy > > > > > > of > > > > > > this > > > > > > + * number also depends on shmem actually swapping > > > > > > out > > > > > > backed-up > > > > > > + * shmem objects without too much buffering. > > > > > > + */ > > > > > > + return (u64)get_nr_swap_pages() << PAGE_SHIFT; > > > > > > +} > > > > > > +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); > > > > > > + > > > > > > +/** > > > > > > + * ttm_backup_shmem_create() - Create a shmem-based struct > > > > > > backup. > > > > > > + * @size: The maximum size (in bytes) to back up. > > > > > > + * > > > > > > + * Create a backup utilizing shmem objects. > > > > > > + * > > > > > > + * Return: A pointer to a struct ttm_backup on success, > > > > > > + * an error pointer on error. > > > > > > + */ > > > > > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size) > > > > > > +{ > > > > > > + struct file *filp; > > > > > > + > > > > > > + filp = shmem_file_setup("ttm shmem backup", size, > > > > > > 0); > > > > > > + > > > > > > + return ttm_file_to_backup(filp); > > > > > > +} > > > > > > diff --git a/include/drm/ttm/ttm_backup.h > > > > > > b/include/drm/ttm/ttm_backup.h > > > > > > new file mode 100644 > > > > > > index 000000000000..20609da7e281 > > > > > > --- /dev/null > > > > > > +++ b/include/drm/ttm/ttm_backup.h > > > > > > @@ -0,0 +1,74 @@ > > > > > > +/* SPDX-License-Identifier: MIT */ > > > > > > +/* > > > > > > + * Copyright © 2024 Intel Corporation > > > > > > + */ > > > > > > + > > > > > > +#ifndef _TTM_BACKUP_H_ > > > > > > +#define _TTM_BACKUP_H_ > > > > > > + > > > > > > +#include <linux/mm_types.h> > > > > > > +#include <linux/shmem_fs.h> > > > > > > + > > > > > > +struct ttm_backup; > > > > > > + > > > > > > +/** > > > > > > + * ttm_backup_handle_to_page_ptr() - Convert handle to > > > > > > struct > > > > > > page > > > > > > pointer > > > > > > + * @handle: The handle to convert. > > > > > > + * > > > > > > + * Converts an opaque handle received from the > > > > > > + * struct ttm_backoup_ops::backup_page() function to an > > > > > > (invalid) > > > > > > + * struct page pointer suitable for a struct page array. > > > > > > + * > > > > > > + * Return: An (invalid) struct page pointer. > > > > > > + */ > > > > > > +static inline struct page * > > > > > > +ttm_backup_handle_to_page_ptr(unsigned long handle) > > > > > > +{ > > > > > > + return (struct page *)(handle << 1 | 1); > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * ttm_backup_page_ptr_is_handle() - Whether a struct page > > > > > > pointer > > > > > > is a handle > > > > > > + * @page: The struct page pointer to check. > > > > > > + * > > > > > > + * Return: true if the struct page pointer is a handld > > > > > > returned > > > > > > from > > > > > > + * ttm_backup_handle_to_page_ptr(). False otherwise. > > > > > > + */ > > > > > > +static inline bool ttm_backup_page_ptr_is_handle(const > > > > > > struct > > > > > > page > > > > > > *page) > > > > > > +{ > > > > > > + return (unsigned long)page & 1; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * ttm_backup_page_ptr_to_handle() - Convert a struct page > > > > > > pointer > > > > > > to a handle > > > > > > + * @page: The struct page pointer to convert > > > > > > + * > > > > > > + * Return: The handle that was previously used in > > > > > > + * ttm_backup_handle_to_page_ptr() to obtain a struct page > > > > > > pointer, suitable > > > > > > + * for use as argument in the struct ttm_backup_ops drop() > > > > > > or > > > > > > + * copy_backed_up_page() functions. > > > > > > + */ > > > > > > +static inline unsigned long > > > > > > +ttm_backup_page_ptr_to_handle(const struct page *page) > > > > > > +{ > > > > > > + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); > > > > > > + return (unsigned long)page >> 1; > > > > > > +} > > > > > > + > > > > > > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t > > > > > > handle); > > > > > > + > > > > > > +int ttm_backup_copy_page(struct ttm_backup *backup, struct > > > > > > page > > > > > > *dst, > > > > > > + pgoff_t handle, bool intr); > > > > > > + > > > > > > +unsigned long > > > > > > +ttm_backup_backup_page(struct ttm_backup *backup, struct > > > > > > page > > > > > > *page, > > > > > > + bool writeback, pgoff_t idx, gfp_t > > > > > > page_gfp, > > > > > > + gfp_t alloc_gfp); > > > > > > + > > > > > > +void ttm_backup_fini(struct ttm_backup *backup); > > > > > > + > > > > > > +u64 ttm_backup_bytes_avail(void); > > > > > > + > > > > > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size); > > > > > > + > > > > > > +#endif >
On Wed, 2024-11-20 at 10:24 +0100, Christian König wrote: > [SNIP] > > > Just that I sleep better: This can never return a folio larger > > > than a > > > page, doesn't it? > > The interface definitely allows for returning larger folios, but > > the > > individual page in the folio is selected by folio_file_page(folio, > > idx). > > Ah, yeah completely missed that and was really wondering why that > would > work. One remaining sligtht concern, though, is that if we repeatedly call - >writepage() for *each* page in a folio, that might degrade performance. Not sure if the filesystem is supposed to coalesce such requests if they happen, or whether things will start to crawl. Right now we can't really tell, since shmem typically only allocates single page folios (unless told otherwise, like i915 does), and in addition shmem writepage() also splits any large folios. I've seen patches floating around to remove that split, though. /Thomas > > > > > /Thomas > > > > > > > Apart from those background questions looks good to me. > > > > > > Regards, > > > Christian. > > > > > > > + > > > > + folio_mark_accessed(to_folio); > > > > + folio_lock(to_folio); > > > > + folio_mark_dirty(to_folio); > > > > + copy_highpage(folio_file_page(to_folio, idx), page); > > > > + handle = ttm_backup_shmem_idx_to_handle(idx); > > > > + > > > > + if (writeback && !folio_mapped(to_folio) && > > > > + folio_clear_dirty_for_io(to_folio)) { > > > > + struct writeback_control wbc = { > > > > + .sync_mode = WB_SYNC_NONE, > > > > + .nr_to_write = SWAP_CLUSTER_MAX, > > > > + .range_start = 0, > > > > + .range_end = LLONG_MAX, > > > > + .for_reclaim = 1, > > > > + }; > > > > + folio_set_reclaim(to_folio); > > > > + ret = mapping->a_ops- > > > > > writepage(folio_file_page(to_folio, idx), &wbc); > > > > + if (!folio_test_writeback(to_folio)) > > > > + folio_clear_reclaim(to_folio); > > > > + /* If writepage succeeds, it unlocks the folio > > > > */ > > > > + if (ret) > > > > + folio_unlock(to_folio); > > The code ignores the error and potentially deserves an explanation > for that. > > Regards, > Christian. > > > > > + } else { > > > > + folio_unlock(to_folio); > > > > + } > > > > + > > > > + folio_put(to_folio); > > > > + > > > > + return handle; > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_fini() - Free the struct backup resources after > > > > last > > > > use. > > > > + * @backup: Pointer to the struct backup whose resources to > > > > free. > > > > + * > > > > + * After a call to this function, it's illegal to use the > > > > @backup > > > > pointer. > > > > + */ > > > > +void ttm_backup_fini(struct ttm_backup *backup) > > > > +{ > > > > + fput(ttm_backup_to_file(backup)); > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_bytes_avail() - Report the approximate number of > > > > bytes of backup space > > > > + * left for backup. > > > > + * > > > > + * This function is intended also for driver use to indicate > > > > whether a > > > > + * backup attempt is meaningful. > > > > + * > > > > + * Return: An approximate size of backup space available. > > > > + */ > > > > +u64 ttm_backup_bytes_avail(void) > > > > +{ > > > > + /* > > > > + * The idea behind backing up to shmem is that shmem > > > > objects may > > > > + * eventually be swapped out. So no point swapping out > > > > if > > > > there > > > > + * is no or low swap-space available. But the accuracy > > > > of > > > > this > > > > + * number also depends on shmem actually swapping out > > > > backed-up > > > > + * shmem objects without too much buffering. > > > > + */ > > > > + return (u64)get_nr_swap_pages() << PAGE_SHIFT; > > > > +} > > > > +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); > > > > + > > > > +/** > > > > + * ttm_backup_shmem_create() - Create a shmem-based struct > > > > backup. > > > > + * @size: The maximum size (in bytes) to back up. > > > > + * > > > > + * Create a backup utilizing shmem objects. > > > > + * > > > > + * Return: A pointer to a struct ttm_backup on success, > > > > + * an error pointer on error. > > > > + */ > > > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size) > > > > +{ > > > > + struct file *filp; > > > > + > > > > + filp = shmem_file_setup("ttm shmem backup", size, 0); > > > > + > > > > + return ttm_file_to_backup(filp); > > > > +} > > > > diff --git a/include/drm/ttm/ttm_backup.h > > > > b/include/drm/ttm/ttm_backup.h > > > > new file mode 100644 > > > > index 000000000000..20609da7e281 > > > > --- /dev/null > > > > +++ b/include/drm/ttm/ttm_backup.h > > > > @@ -0,0 +1,74 @@ > > > > +/* SPDX-License-Identifier: MIT */ > > > > +/* > > > > + * Copyright © 2024 Intel Corporation > > > > + */ > > > > + > > > > +#ifndef _TTM_BACKUP_H_ > > > > +#define _TTM_BACKUP_H_ > > > > + > > > > +#include <linux/mm_types.h> > > > > +#include <linux/shmem_fs.h> > > > > + > > > > +struct ttm_backup; > > > > + > > > > +/** > > > > + * ttm_backup_handle_to_page_ptr() - Convert handle to struct > > > > page > > > > pointer > > > > + * @handle: The handle to convert. > > > > + * > > > > + * Converts an opaque handle received from the > > > > + * struct ttm_backoup_ops::backup_page() function to an > > > > (invalid) > > > > + * struct page pointer suitable for a struct page array. > > > > + * > > > > + * Return: An (invalid) struct page pointer. > > > > + */ > > > > +static inline struct page * > > > > +ttm_backup_handle_to_page_ptr(unsigned long handle) > > > > +{ > > > > + return (struct page *)(handle << 1 | 1); > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_page_ptr_is_handle() - Whether a struct page > > > > pointer > > > > is a handle > > > > + * @page: The struct page pointer to check. > > > > + * > > > > + * Return: true if the struct page pointer is a handld > > > > returned > > > > from > > > > + * ttm_backup_handle_to_page_ptr(). False otherwise. > > > > + */ > > > > +static inline bool ttm_backup_page_ptr_is_handle(const struct > > > > page > > > > *page) > > > > +{ > > > > + return (unsigned long)page & 1; > > > > +} > > > > + > > > > +/** > > > > + * ttm_backup_page_ptr_to_handle() - Convert a struct page > > > > pointer > > > > to a handle > > > > + * @page: The struct page pointer to convert > > > > + * > > > > + * Return: The handle that was previously used in > > > > + * ttm_backup_handle_to_page_ptr() to obtain a struct page > > > > pointer, suitable > > > > + * for use as argument in the struct ttm_backup_ops drop() or > > > > + * copy_backed_up_page() functions. > > > > + */ > > > > +static inline unsigned long > > > > +ttm_backup_page_ptr_to_handle(const struct page *page) > > > > +{ > > > > + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); > > > > + return (unsigned long)page >> 1; > > > > +} > > > > + > > > > +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t > > > > handle); > > > > + > > > > +int ttm_backup_copy_page(struct ttm_backup *backup, struct > > > > page > > > > *dst, > > > > + pgoff_t handle, bool intr); > > > > + > > > > +unsigned long > > > > +ttm_backup_backup_page(struct ttm_backup *backup, struct page > > > > *page, > > > > + bool writeback, pgoff_t idx, gfp_t > > > > page_gfp, > > > > + gfp_t alloc_gfp); > > > > + > > > > +void ttm_backup_fini(struct ttm_backup *backup); > > > > + > > > > +u64 ttm_backup_bytes_avail(void); > > > > + > > > > +struct ttm_backup *ttm_backup_shmem_create(loff_t size); > > > > + > > > > +#endif
diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile index dad298127226..40d07a35293a 100644 --- a/drivers/gpu/drm/ttm/Makefile +++ b/drivers/gpu/drm/ttm/Makefile @@ -4,7 +4,7 @@ ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \ ttm_execbuf_util.o ttm_range_manager.o ttm_resource.o ttm_pool.o \ - ttm_device.o ttm_sys_manager.o + ttm_device.o ttm_sys_manager.o ttm_backup.o ttm-$(CONFIG_AGP) += ttm_agp_backend.o obj-$(CONFIG_DRM_TTM) += ttm.o diff --git a/drivers/gpu/drm/ttm/ttm_backup.c b/drivers/gpu/drm/ttm/ttm_backup.c new file mode 100644 index 000000000000..bf16bb0c594e --- /dev/null +++ b/drivers/gpu/drm/ttm/ttm_backup.c @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include <drm/ttm/ttm_backup.h> +#include <linux/page-flags.h> +#include <linux/swap.h> + +/* + * Casting from randomized struct file * to struct ttm_backup * is fine since + * struct ttm_backup is never defined nor dereferenced. + */ +static struct file *ttm_backup_to_file(struct ttm_backup *backup) +{ + return (void *)backup; +} + +static struct ttm_backup *ttm_file_to_backup(struct file *file) +{ + return (void *)file; +} + +/* + * Need to map shmem indices to handle since a handle value + * of 0 means error, following the swp_entry_t convention. + */ +static unsigned long ttm_backup_shmem_idx_to_handle(pgoff_t idx) +{ + return (unsigned long)idx + 1; +} + +static pgoff_t ttm_backup_handle_to_shmem_idx(pgoff_t handle) +{ + return handle - 1; +} + +/** + * ttm_backup_drop() - release memory associated with a handle + * @backup: The struct backup pointer used to obtain the handle + * @handle: The handle obtained from the @backup_page function. + */ +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle) +{ + loff_t start = ttm_backup_handle_to_shmem_idx(handle); + + start <<= PAGE_SHIFT; + shmem_truncate_range(file_inode(ttm_backup_to_file(backup)), start, + start + PAGE_SIZE - 1); +} + +/** + * ttm_backup_copy_page() - Copy the contents of a previously backed + * up page + * @backup: The struct backup pointer used to back up the page. + * @dst: The struct page to copy into. + * @handle: The handle returned when the page was backed up. + * @intr: Try to perform waits interruptable or at least killable. + * + * Return: 0 on success, Negative error code on failure, notably + * -EINTR if @intr was set to true and a signal is pending. + */ +int ttm_backup_copy_page(struct ttm_backup *backup, struct page *dst, + pgoff_t handle, bool intr) +{ + struct file *filp = ttm_backup_to_file(backup); + struct address_space *mapping = filp->f_mapping; + struct folio *from_folio; + pgoff_t idx = ttm_backup_handle_to_shmem_idx(handle); + + from_folio = shmem_read_folio(mapping, idx); + if (IS_ERR(from_folio)) + return PTR_ERR(from_folio); + + copy_highpage(dst, folio_file_page(from_folio, idx)); + folio_put(from_folio); + + return 0; +} + +/** + * ttm_backup_backup_page() - Backup a page + * @backup: The struct backup pointer to use. + * @page: The page to back up. + * @writeback: Whether to perform immediate writeback of the page. + * This may have performance implications. + * @idx: A unique integer for each page and each struct backup. + * This allows the backup implementation to avoid managing + * its address space separately. + * @page_gfp: The gfp value used when the page was allocated. + * This is used for accounting purposes. + * @alloc_gfp: The gpf to be used when allocating memory. + * + * Context: If called from reclaim context, the caller needs to + * assert that the shrinker gfp has __GFP_FS set, to avoid + * deadlocking on lock_page(). If @writeback is set to true and + * called from reclaim context, the caller also needs to assert + * that the shrinker gfp has __GFP_IO set, since without it, + * we're not allowed to start backup IO. + * + * Return: A handle on success. 0 on failure. + * (This is following the swp_entry_t convention). + * + * Note: This function could be extended to back up a folio and + * implementations would then split the folio internally if needed. + * Drawback is that the caller would then have to keep track of + * the folio size- and usage. + */ +unsigned long +ttm_backup_backup_page(struct ttm_backup *backup, struct page *page, + bool writeback, pgoff_t idx, gfp_t page_gfp, + gfp_t alloc_gfp) +{ + struct file *filp = ttm_backup_to_file(backup); + struct address_space *mapping = filp->f_mapping; + unsigned long handle = 0; + struct folio *to_folio; + int ret; + + to_folio = shmem_read_folio_gfp(mapping, idx, alloc_gfp); + if (IS_ERR(to_folio)) + return handle; + + folio_mark_accessed(to_folio); + folio_lock(to_folio); + folio_mark_dirty(to_folio); + copy_highpage(folio_file_page(to_folio, idx), page); + handle = ttm_backup_shmem_idx_to_handle(idx); + + if (writeback && !folio_mapped(to_folio) && + folio_clear_dirty_for_io(to_folio)) { + struct writeback_control wbc = { + .sync_mode = WB_SYNC_NONE, + .nr_to_write = SWAP_CLUSTER_MAX, + .range_start = 0, + .range_end = LLONG_MAX, + .for_reclaim = 1, + }; + folio_set_reclaim(to_folio); + ret = mapping->a_ops->writepage(folio_file_page(to_folio, idx), &wbc); + if (!folio_test_writeback(to_folio)) + folio_clear_reclaim(to_folio); + /* If writepage succeeds, it unlocks the folio */ + if (ret) + folio_unlock(to_folio); + } else { + folio_unlock(to_folio); + } + + folio_put(to_folio); + + return handle; +} + +/** + * ttm_backup_fini() - Free the struct backup resources after last use. + * @backup: Pointer to the struct backup whose resources to free. + * + * After a call to this function, it's illegal to use the @backup pointer. + */ +void ttm_backup_fini(struct ttm_backup *backup) +{ + fput(ttm_backup_to_file(backup)); +} + +/** + * ttm_backup_bytes_avail() - Report the approximate number of bytes of backup space + * left for backup. + * + * This function is intended also for driver use to indicate whether a + * backup attempt is meaningful. + * + * Return: An approximate size of backup space available. + */ +u64 ttm_backup_bytes_avail(void) +{ + /* + * The idea behind backing up to shmem is that shmem objects may + * eventually be swapped out. So no point swapping out if there + * is no or low swap-space available. But the accuracy of this + * number also depends on shmem actually swapping out backed-up + * shmem objects without too much buffering. + */ + return (u64)get_nr_swap_pages() << PAGE_SHIFT; +} +EXPORT_SYMBOL_GPL(ttm_backup_bytes_avail); + +/** + * ttm_backup_shmem_create() - Create a shmem-based struct backup. + * @size: The maximum size (in bytes) to back up. + * + * Create a backup utilizing shmem objects. + * + * Return: A pointer to a struct ttm_backup on success, + * an error pointer on error. + */ +struct ttm_backup *ttm_backup_shmem_create(loff_t size) +{ + struct file *filp; + + filp = shmem_file_setup("ttm shmem backup", size, 0); + + return ttm_file_to_backup(filp); +} diff --git a/include/drm/ttm/ttm_backup.h b/include/drm/ttm/ttm_backup.h new file mode 100644 index 000000000000..20609da7e281 --- /dev/null +++ b/include/drm/ttm/ttm_backup.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2024 Intel Corporation + */ + +#ifndef _TTM_BACKUP_H_ +#define _TTM_BACKUP_H_ + +#include <linux/mm_types.h> +#include <linux/shmem_fs.h> + +struct ttm_backup; + +/** + * ttm_backup_handle_to_page_ptr() - Convert handle to struct page pointer + * @handle: The handle to convert. + * + * Converts an opaque handle received from the + * struct ttm_backoup_ops::backup_page() function to an (invalid) + * struct page pointer suitable for a struct page array. + * + * Return: An (invalid) struct page pointer. + */ +static inline struct page * +ttm_backup_handle_to_page_ptr(unsigned long handle) +{ + return (struct page *)(handle << 1 | 1); +} + +/** + * ttm_backup_page_ptr_is_handle() - Whether a struct page pointer is a handle + * @page: The struct page pointer to check. + * + * Return: true if the struct page pointer is a handld returned from + * ttm_backup_handle_to_page_ptr(). False otherwise. + */ +static inline bool ttm_backup_page_ptr_is_handle(const struct page *page) +{ + return (unsigned long)page & 1; +} + +/** + * ttm_backup_page_ptr_to_handle() - Convert a struct page pointer to a handle + * @page: The struct page pointer to convert + * + * Return: The handle that was previously used in + * ttm_backup_handle_to_page_ptr() to obtain a struct page pointer, suitable + * for use as argument in the struct ttm_backup_ops drop() or + * copy_backed_up_page() functions. + */ +static inline unsigned long +ttm_backup_page_ptr_to_handle(const struct page *page) +{ + WARN_ON(!ttm_backup_page_ptr_is_handle(page)); + return (unsigned long)page >> 1; +} + +void ttm_backup_drop(struct ttm_backup *backup, pgoff_t handle); + +int ttm_backup_copy_page(struct ttm_backup *backup, struct page *dst, + pgoff_t handle, bool intr); + +unsigned long +ttm_backup_backup_page(struct ttm_backup *backup, struct page *page, + bool writeback, pgoff_t idx, gfp_t page_gfp, + gfp_t alloc_gfp); + +void ttm_backup_fini(struct ttm_backup *backup); + +u64 ttm_backup_bytes_avail(void); + +struct ttm_backup *ttm_backup_shmem_create(loff_t size); + +#endif