From mboxrd@z Thu Jan 1 00:00:00 1970 References: <20160114183423.69665622@md1em3qc> <1453821607-20836-1-git-send-email-henning.schild@siemens.com> <56A7D4AD.9090601@siemens.com> <20160127105448.65c467d2@md1em3qc> <56A89C77.9050105@siemens.com> From: Philippe Gerum Message-ID: <56A89FA8.3010306@xenomai.org> Date: Wed, 27 Jan 2016 11:44:56 +0100 MIME-Version: 1.0 In-Reply-To: <56A89C77.9050105@siemens.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai] [PATCH] ipipe x86 mm: handle huge pages in memory pinning List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka , Henning Schild Cc: Xenomai On 01/27/2016 11:31 AM, Jan Kiszka wrote: > On 2016-01-27 10:54, Henning Schild wrote: >> On Tue, 26 Jan 2016 21:18:53 +0100 >> Jan Kiszka wrote: >> >>> On 2016-01-26 16:20, Henning Schild wrote: >>>> In 4.1 huge page mapping of io memory was introduced, enable ipipe >>>> to handle that when pinning kernel memory. >>>> >>>> change that introduced the feature >>>> 0f616be120c632c818faaea9adcb8f05a7a8601f >>>> >>>> Signed-off-by: Henning Schild >>>> --- >>>> arch/x86/mm/fault.c | 16 ++++++++++++++++ >>>> 1 file changed, 16 insertions(+) >>>> >>>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c >>>> index fd5bbcc..5585610 100644 >>>> --- a/arch/x86/mm/fault.c >>>> +++ b/arch/x86/mm/fault.c >>>> @@ -211,11 +211,19 @@ static inline pmd_t *vmalloc_sync_one(pgd_t >>>> *pgd, unsigned long address) pud_k = pud_offset(pgd_k, address); >>>> if (!pud_present(*pud_k)) >>>> return NULL; >>>> +#if defined(CONFIG_IPIPE) && defined(CONFIG_HAVE_ARCH_HUGE_VMAP) >>>> + if (pud_large(*pud)) >>>> + return pud_k; >>>> +#endif >>>> >>>> pmd = pmd_offset(pud, address); >>>> pmd_k = pmd_offset(pud_k, address); >>>> if (!pmd_present(*pmd_k)) >>>> return NULL; >>>> +#if defined(CONFIG_IPIPE) && defined(CONFIG_HAVE_ARCH_HUGE_VMAP) >>>> + if (pmd_large(*pmd)) >>>> + return pmd_k; >>>> +#endif >>>> >>>> if (!pmd_present(*pmd)) >>>> set_pmd(pmd, *pmd_k); >>>> @@ -400,6 +408,10 @@ static inline int vmalloc_sync_one(pgd_t *pgd, >>>> unsigned long address) >>>> if (pud_none(*pud) || pud_page_vaddr(*pud) != >>>> pud_page_vaddr(*pud_ref)) BUG(); >>>> +#if defined(CONFIG_IPIPE) && defined(CONFIG_HAVE_ARCH_HUGE_VMAP) >>>> + if (pud_large(*pud)) >>>> + return 0; >>>> +#endif >>>> >>>> pmd = pmd_offset(pud, address); >>>> pmd_ref = pmd_offset(pud_ref, address); >>>> @@ -408,6 +420,10 @@ static inline int vmalloc_sync_one(pgd_t *pgd, >>>> unsigned long address) >>>> if (pmd_none(*pmd) || pmd_page(*pmd) != pmd_page(*pmd_ref)) >>>> BUG(); >>>> +#if defined(CONFIG_IPIPE) && defined(CONFIG_HAVE_ARCH_HUGE_VMAP) >>>> + if (pmd_large(*pmd)) >>>> + return 0; >>>> +#endif >>>> >>>> pte_ref = pte_offset_kernel(pmd_ref, address); >>>> if (!pte_present(*pte_ref)) >>>> >>> >>> Do we need the #ifdefs? If not, maybe just leave comments to establish >>> the relationship to I-pipe. >> >> We do not, without ipipe the function is never called from a >> context where that check is required. If it was, this change or >> something similar would be in mainline. >> I see the ifdefs as kind of comments that establish the relationship to >> ipipe and the huge_vmap feature. Plus in all other cases a pointless >> check is not even compiled in. >> If comments should be used instead of ifdefs i will change that. > > The optimal way of documenting the motivation for these changes is in a > changelog. Provided the patch will survive future I-pipe versions and > not be folded into a bigger patch (Philippe, please confirm), In the current workflow, only raw/* branches may be rebased, so folding patches within a maintenance branch can't happen. let's go > for that. Plan B is inline comments. But #ifdefs for documentation > purposes are not desirable. > Documentation is best when close to the documented code, so to me plan B is actually plan A, although I agree that complex/tricky/non-obvious matters addressed by any given patch should be described accurately in its changelog. -- Philippe.