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
