Date:	Thu, 17 Sep 2009 00:11:33 +1200
From:	Alexey Korolev <akorolex@...il.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	Eric Munson <linux-mm@...bm.net>,
	Alexey Korolev <akorolev@...radead.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] Identification of huge pages mapping (Take 3)

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists