On Thu, Mar 24, 2016 at 5:41 PM, Joonas Lahtinen
<joonas.lahtinen@xxxxxxxxxxxxxxx> wrote:
> On ke, 2016-03-23 at 11:39 +0530, akash.goel@xxxxxxxxx wrote:
>> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>
>> This provides support for the Drivers or shmem file owners to register
>> a set of callbacks, which can be invoked from the address space operations
>> methods implemented by shmem.
>> This allow the file owners to hook into the shmem address space operations
>> to do some extra/custom operations in addition to the default ones.
>>
>> The private_data field of address_space struct is used to store the pointer
>> to driver specific ops.
>> Currently only one ops field is defined, which is migratepage, but can be
>> extended on need basis.
>>
>> The need for driver specific operations arises since some of the operations
>> (like migratepage) may not be handled completely within shmem, so as to be
>> effective, and would need some driver specific handling also.
>>
>> Specifically, i915.ko would like to participate in migratepage().
>> i915.ko uses shmemfs to provide swappable backing storage for its user
>> objects, but when those objects are in use by the GPU it must pin the entire
>> object until the GPU is idle. As a result, large chunks of memory can be
>> arbitrarily withdrawn from page migration, resulting in premature
>> out-of-memory due to fragmentation. However, if i915.ko can receive the
>> migratepage() request, it can then flush the object from the GPU, remove
>> its pin and thus enable the migration.
>>
>> Since Gfx allocations are one of the major consumer of system memory, its
>> imperative to have such a mechanism to effectively deal with fragmentation.
>> And therefore the need for such a provision for initiating driver specific
>> actions during address space operations.
>>
>> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
>> Cc: linux-mm@xxxxxxxxx
>> Signed-off-by: Sourab Gupta <sourab.gupta@xxxxxxxxx>
>> Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
>> ---
>> include/linux/shmem_fs.h | 17 +++++++++++++++++
>> mm/shmem.c | 17 ++++++++++++++++-
>> 2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 4d4780c..6cfa76a 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -34,11 +34,28 @@ struct shmem_sb_info {
>> struct mempolicy *mpol; /* default memory policy for mappings */
>> };
>>
>> +struct shmem_dev_info {
>> + void *dev_private_data;
>> + int (*dev_migratepage)(struct address_space *mapping,
>> + struct page *newpage, struct page *page,
>> + enum migrate_mode mode, void *dev_priv_data);
>
> One might want to have a separate shmem_dev_operations struct or
> similar.
>
Sorry for the very late turnaround.
Sorry couldn't get your point here. Are you suggesting to rename the
structure to shmem_dev_operations ?
>> +};
>> +
>> static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
>> {
>> return container_of(inode, struct shmem_inode_info, vfs_inode);
>> }
>>
>> +static inline int shmem_set_device_ops(struct address_space *mapping,
>> + struct shmem_dev_info *info)
>> +{
>> + if (mapping->private_data != NULL)
>> + return -EEXIST;
>> +
>
> I did a quick random peek and most set functions are just void and
> override existing data. I'd suggest the same.
>
>> + mapping->private_data = info;
>
Fine will change the return type to void and remove the check.
> Also, doesn't this kinda steal the mapping->private_data, might that be
> unexpected for the user? I notice currently it's not being touched at
> all.
>
Sorry by User do you mean the shmem client who called shmem_file_setup() ?
It seems clients are not expected to touch mapping->private_data and
so shmemfs can safely use it.
Best regards
Akash
>> + return 0;
>> +}
>> +
>> /*
>> * Functions in mm/shmem.c called directly from elsewhere:
>> */
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 440e2a7..f8625c4 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -952,6 +952,21 @@ redirty:
>> return 0;
>> }
>>
>> +#ifdef CONFIG_MIGRATION
>> +static int shmem_migratepage(struct address_space *mapping,
>> + struct page *newpage, struct page *page,
>> + enum migrate_mode mode)
>> +{
>> + struct shmem_dev_info *dev_info = mapping->private_data;
>> +
>> + if (dev_info && dev_info->dev_migratepage)
>> + return dev_info->dev_migratepage(mapping, newpage, page,
>> + mode, dev_info->dev_private_data);
>> +
>> + return migrate_page(mapping, newpage, page, mode);
>> +}
>> +#endif
>> +
>> #ifdef CONFIG_NUMA
>> #ifdef CONFIG_TMPFS
>> static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> @@ -3168,7 +3183,7 @@ static const struct address_space_operations shmem_aops = {
>> .write_end = shmem_write_end,
>> #endif
>> #ifdef CONFIG_MIGRATION
>> - .migratepage = migrate_page,
>> + .migratepage = shmem_migratepage,
>> #endif
>> .error_remove_page = generic_error_remove_page,
>> };
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
