From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsZfo-0003gc-Eq for qemu-devel@nongnu.org; Sat, 31 Oct 2015 13:10:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZsZfj-0005Wg-50 for qemu-devel@nongnu.org; Sat, 31 Oct 2015 13:10:36 -0400 Received: from mga14.intel.com ([192.55.52.115]:14427) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsZfi-0005Wc-Uv for qemu-devel@nongnu.org; Sat, 31 Oct 2015 13:10:31 -0400 References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-8-git-send-email-guangrong.xiao@linux.intel.com> <20151030155454.GS4180@thinpad.lan.raisama.net> <56347754.6090904@linux.intel.com> <20151031141131.GX4180@thinpad.lan.raisama.net> From: Xiao Guangrong Message-ID: <5634F463.6070001@linux.intel.com> Date: Sun, 1 Nov 2015 01:03:31 +0800 MIME-Version: 1.0 In-Reply-To: <20151031141131.GX4180@thinpad.lan.raisama.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 07/33] util: introduce qemu_file_get_page_size() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 10/31/2015 10:11 PM, Eduardo Habkost wrote: > On Sat, Oct 31, 2015 at 04:09:56PM +0800, Xiao Guangrong wrote: >> On 10/30/2015 11:54 PM, Eduardo Habkost wrote: >>> On Fri, Oct 30, 2015 at 01:56:01PM +0800, Xiao Guangrong wrote: >>>> There are three places use the some logic to get the page size on >>>> the file path or file fd >>>> >>>> This patch introduces qemu_file_get_page_size() to unify the code >>>> >>>> Signed-off-by: Xiao Guangrong > [...] >>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >>>> index ac70f08..c661f1c 100644 >>>> --- a/target-ppc/kvm.c >>>> +++ b/target-ppc/kvm.c >>>> @@ -308,28 +308,13 @@ static void kvm_get_smmu_info(PowerPCCPU *cpu, struct kvm_ppc_smmu_info *info) >>>> >>>> static long gethugepagesize(const char *mem_path) >>>> { >>>> - struct statfs fs; >>>> - int ret; >>>> - >>>> - do { >>>> - ret = statfs(mem_path, &fs); >>>> - } while (ret != 0 && errno == EINTR); >>>> + long size = qemu_file_get_page_size(mem_path); >>>> >>>> - if (ret != 0) { >>>> - fprintf(stderr, "Couldn't statfs() memory path: %s\n", >>>> - strerror(errno)); >>>> + if (!size) { >>>> exit(1); >>>> } >>>> >>>> -#define HUGETLBFS_MAGIC 0x958458f6 >>>> - >>>> - if (fs.f_type != HUGETLBFS_MAGIC) { >>>> - /* Explicit mempath, but it's ordinary pages */ >>>> - return getpagesize(); >>>> - } >>>> - >>>> - /* It's hugepage, return the huge page size */ >>>> - return fs.f_bsize; >>>> + return size; >>>> } >>> >>> Why are you changing target-ppc/kvm.c:gethugepagesize() to use the new >>> funtion, but not the copy at exec.c? To make it simpler, we could >>> eliminate both gethugepagesize() functions completely and replace them >>> with qemu_file_get_page_size() calls (maybe as part of this patch, maybe >>> in a separate patch, I'm not sure). >>> >> >> The gethugepagesize() in exec.c will be eliminated in later patch :). > > That's true. I was just expecting to see the change that eliminates > gethugepagesize() in exec.c here instead of patch 08/33. Simple cleanups > that just eliminate code duplication without change in behavior are > easier to review and more likely to be included before the rest of the > series. Completely agree, will move the replacement from 08/33 to here in the next version. > > >> >> And the gethugepagesize() in ppc platform has error handling logic >> and has multiple caller. It's not so bad to keep it. > > Well, in case there's still room for cleanup in the ppc code, it may be > done later. No problem. > Thank you, Eduardo!