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
