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
>
