Re: [PATCH 1/3] Identification of huge pages mapping (Take 3)

[Alexey Korolev]

Mel,

> I suggest a subject change to
>
> "Identify huge page mappings from address_space->flags instead of file_operations comparison"
>
> for the purposes of having an easier-to-understand changelog.
>
Yes. It is a bit longer but it is definitely clear. Will be corrected.

> On Mon, Sep 14, 2009 at 05:16:13PM +1200, Alexey Korolev wrote:
>> This patch changes a little bit the procedures of huge pages file
>> identification. We need this because we may have huge page mapping for
>> files which are not on hugetlbfs (the same case in ipc/shm.c).
>
> Is this strictly-speaking true as there is still a file on hugetlbfs for
> the driver? Maybe something like
>
> This patch identifies whether a mapping uses huge pages based on the
> address_space flags instead of the file operations. A later patch allows
> a driver to manage an underlying hugetlbfs file while exposing it via a
> different file_operations structure.
>
> I haven't read the rest of the series yet so take the suggestion with a
> grain of salt.

You understood properly. Thanks for the comments. I need to work on
the description more, it seems not to be completely clear.

>> Just file operations check will not work as drivers should have own
>> file operations. So if we need to identify if file has huge pages
>> mapping, we need to check the file mapping flags.
>> New identification procedure obsoletes existing workaround for hugetlb
>> file identification in ipc/shm.c
>> Also having huge page mapping for files which are not on hugetlbfs do
>> not allow us to get hstate based on file dentry, we need to be based
>> on file mapping instead.
>
> Can you clarify this a bit more? I think the reasoning is as follows but
> confirmation would be nice.
>
> "As part of this, the hstate for a given file as implemented by hstate_file()
> must be based on file mapping instead of dentry. Even if a driver is
> maintaining an underlying hugetlbfs file, the mmap() operation is still
> taking place on a device-specific file. That dentry is unlikely to be on
> a hugetlbfs file. A device driver must ensure that file->f_mapping->host
> resolves correctly."
>
> If this is accurate, a comment in hstate_file() wouldn't hurt in case
> someone later decides that dentry really was the way to go.
>
Right. Getting hstate via mapping instead of dentry is important here, so it is
necessary to add a comment in order to prevent people breaking this.
A comment will be added.

>>
>> Âstatic inline int is_file_hugepages(struct file *file)
>> Â{
>> - Â Â if (file->f_op == &hugetlbfs_file_operations)
>> - Â Â Â Â Â Â return 1;
>> - Â Â if (is_file_shm_hugepages(file))
>> - Â Â Â Â Â Â return 1;
>> -
>> - Â Â return 0;
>> -}
>> -
>> -static inline void set_file_hugepages(struct file *file)
>> -{
>> - Â Â file->f_op = &hugetlbfs_file_operations;
>> + Â Â return mapping_hugetlb(file->f_mapping);
>> Â}
>> Â#else /* !CONFIG_HUGETLBFS */
>>
>> Â#define is_file_hugepages(file) Â Â Â Â Â Â Â Â Â Â Â0
>> -#define set_file_hugepages(file) Â Â Â Â Â Â BUG()
>> Â#define hugetlb_file_setup(name,size,acct,user,creat) Â Â Â ÂERR_PTR(-ENOSYS)
>>
>
> Why do you remove this BUG()? It still seems to be a valid check.
I removed this function - because it has not been called since 2.6.15 and
it is confusing the user a bit after applying new changes. I think it
was necessary to write about this little change in description, sorry
about that.
>>
>> +static inline void mapping_set_hugetlb(struct address_space *mapping)
>> +{
>> + Â Â set_bit(AS_HUGETLB, &mapping->flags);
>> +}
>> +
>> +static inline int mapping_hugetlb(struct address_space *mapping)
>> +{
>> + Â Â if (likely(mapping))
>> + Â Â Â Â Â Â return test_bit(AS_HUGETLB, &mapping->flags);
>> + Â Â return 0;
>> +}
>
> Is mapping_hugetlb necessary? Why not just make that the implementation
> of is_file_hugepages()
No. It is not necessary. The reason I wrote these functions is just
there are the
similar function for other mapping flags. I see no problem to have
only: is_file_hugepages and
set_file_huge_pages in hugetlb.h instead of mapping_set_hugetlb and
mapping_hugetlb.

>> - Â Â if (file->f_op == &shm_file_operations) {
>> - Â Â Â Â Â Â struct shm_file_data *sfd;
>> - Â Â Â Â Â Â sfd = shm_file_data(file);
>> - Â Â Â Â Â Â ret = is_file_hugepages(sfd->file);
>> - Â Â }
>> - Â Â return ret;
>> -}
>
> What about the declarations and definitions in include/linux/shm.h?

Ahh. Thank you! Will be fixed.

Thanks,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/