From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758472AbZIQJSO (ORCPT ); Thu, 17 Sep 2009 05:18:14 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754757AbZIQJSO (ORCPT ); Thu, 17 Sep 2009 05:18:14 -0400 Received: from gir.skynet.ie ([193.1.99.77]:41687 "EHLO gir.skynet.ie" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751190AbZIQJSM (ORCPT ); Thu, 17 Sep 2009 05:18:12 -0400 Date: Thu, 17 Sep 2009 10:18:18 +0100 From: Mel Gorman To: Alexey Korolev Cc: Eric Munson , Alexey Korolev , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] Identification of huge pages mapping (Take 3) Message-ID: <20090917091818.GC13002@csn.ul.ie> References: <202cde0e0909132216l79aae251ya3a6685587c7692c@mail.gmail.com> <20090915121456.GB31840@csn.ul.ie> <202cde0e0909160511y6f4542d1p38f9a8818c2a454d@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <202cde0e0909160511y6f4542d1p38f9a8818c2a454d@mail.gmail.com> User-Agent: Mutt/1.5.17+20080114 (2008-01-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 17, 2009 at 12:11:33AM +1200, Alexey Korolev wrote: > 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. > Thanks > > 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. > Thanks > >> > >>  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. If it's really confusing, it should be a separate patch. It doesn't need to be folded into this one. > >> > >> +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. > I'd also be ok with you converting is_file_hugepages and set_file_huge_pages() to mapping_set_hugetlb and mapping_hugetlb to bring hugetlb functions more in line with the core VM. > >> -     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 > -- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab