<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Am 20.11.24 um 08:58 schrieb Thomas Hellström: <blockquote type="cite" cite="mid:7f3536a2e436566145215fad8889eee77dfa271c.camel@linux.intel.com"> <pre class="moz-quote-pre" wrap="">On Tue, 2024-11-19 at 14:40 +0100, Christian König wrote: </pre> <blockquote type="cite">[SNIP] <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ +/* + * 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) </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Do I get it right that struct ttm_backup is never really defined? </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Yes. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">What purpose does that have? </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> 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.</pre> </blockquote> That is usually done with a typedef and one of the few cases where typedefs are actually advised to be used. [SNIP] <blockquote type="cite" cite="mid:7f3536a2e436566145215fad8889eee77dfa271c.camel@linux.intel.com"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ * + * 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;</pre> </blockquote> </blockquote> </blockquote> 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? <blockquote type="cite" cite="mid:7f3536a2e436566145215fad8889eee77dfa271c.camel@linux.intel.com"> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Just that I sleep better: This can never return a folio larger than a page, doesn't it? </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> The interface definitely allows for returning larger folios, but the individual page in the folio is selected by folio_file_page(folio, idx).</pre> </blockquote> Ah, yeah completely missed that and was really wondering why that would work. <blockquote type="cite" cite="mid:7f3536a2e436566145215fad8889eee77dfa271c.camel@linux.intel.com"> <pre class="moz-quote-pre" wrap=""> /Thomas </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> Apart from those background questions looks good to me. Regards, Christian. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ + 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- </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">writepage(folio_file_page(to_folio, idx), &wbc); </pre> </blockquote> <pre class="moz-quote-pre" wrap="">+ if (!folio_test_writeback(to_folio)) + folio_clear_reclaim(to_folio); + /* If writepage succeeds, it unlocks the folio */ + if (ret) + folio_unlock(to_folio);</pre> </blockquote> </blockquote> </blockquote> The code ignores the error and potentially deserves an explanation for that. Regards, Christian. <blockquote type="cite" cite="mid:7f3536a2e436566145215fad8889eee77dfa271c.camel@linux.intel.com"> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> + } 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 </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>