[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CS1PR84MB01198F4F6DCE8D35E85394208EAD0@CS1PR84MB0119.NAMPRD84.PROD.OUTLOOK.COM>
Date: Fri, 28 Oct 2016 01:52:47 +0000
From: "Boylston, Brian" <brian.boylston@....com>
To: Thomas Gleixner <tglx@...utronix.de>
CC: "linux-nvdimm@...1.01.org" <linux-nvdimm@...1.01.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Kani, Toshimitsu" <toshi.kani@....com>,
"Moreno, Oliver" <oliver.moreno@....com>,
Ross Zwisler <ross.zwisler@...ux.intel.com>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
"x86@...nel.org" <x86@...nel.org>,
Al Viro <viro@...IV.linux.org.uk>,
Dan Williams <dan.j.williams@...el.com>,
"boylston@...romesa.net" <boylston@...romesa.net>
Subject: RE: [PATCH v2 1/3] introduce memcpy_nocache()
Thomas Gleixner wrote on 2016-10-26:
> On Wed, 26 Oct 2016, Brian Boylston wrote:
>> --- a/arch/x86/include/asm/string_32.h
>> +++ b/arch/x86/include/asm/string_32.h
>> @@ -196,6 +196,9 @@ static inline void *__memcpy3d(void *to, const void *from, size_t len)
>
>> +#define __HAVE_ARCH_MEMCPY_NOCACHE
>> +extern void *memcpy_nocache(void *dest, const void *src, size_t count);
>
> Can we please move that prototype to linux/string.h instead of having it in
> both files and just define __HAVE_ARCH_MEMCPY_NOCACHE? And that define can
> move into x86/include/asm/string.h. There is no point in duplicating all
> that stuff.
Yes, sounds good.
>
>> --- a/arch/x86/include/asm/string_64.h
>> +++ b/arch/x86/include/asm/string_64.h
>> @@ -51,6 +51,9 @@ extern void *__memcpy(void *to, const void *from, size_t len);
>
>> +#define __HAVE_ARCH_MEMCPY_NOCACHE
>> +extern void *memcpy_nocache(void *dest, const void *src, size_t count)
>
>> +#ifdef __HAVE_ARCH_MEMCPY_NOCACHE
>
> You define __HAVE_ARCH_MEMCPY_NOCACHE for both 32 and 64 bit. What is
> this #ifdef for ?
Now that you raise the question, I'm not sure, and I see that the
x86 memcpy() definition isn't similarly wrapped. I can adjust it.
>
>> +void *memcpy_nocache(void *dest, const void *src, size_t count)
>> +{
>> + __copy_from_user_inatomic_nocache(dest, src, count);
>
> You want to replace a memcpy with this and then use copy from user. Why?
> That does not make any sense to me and even if it makes sense for whatever
> reason then this wants to be documented and the src argument needs a type
> cast to (void __user *)..
I agree that the documentation needs improvement. The goal is to memcpy
while avoiding the processor cache. Following x86's arch_memcpy_to_pmem(),
I was thinking that the user nocache implementation on x86 would work,
but I have none of the comments that arch_memcpy_to_pmem() has...
>
> Further this uses the inatomic variant. Why? Does a memcpy replacement
> suddenly require to be called in atomic context? Again, this makes no sense
> and lacks any form of comment. The kernel doc below does not say anything
> about that.
I borrowed the use of the inatomic variant from arch_memcpy_to_pmem().
Perhaps I need to look at memcpy_to_pmem() as suggested by Boaz.
>
> Neither provides the changelog any useful information which is
> complementary to what the patch actually does.
>
>> +#ifndef __HAVE_ARCH_MEMCPY_NOCACHE
>> +/**
>> + * memcpy_nocache - Copy one area of memory to another, avoiding the
>> + * processor cache if possible
>
> I'm not sure whether kerneldoc is happy about that line break. Did you
> build the docs?
>
> Make the initial line shorter and explain the functionality in detail
> below the arguments
>
>> + * @dest: Where to copy to
>> + * @src: Where to copy from
>> + * @count: The size of the area.
>
> Nit. Can you please make this in tabular form for readability sake?
No, I did not try to build the docs. I'll revisit this and your other
doc-related feedback.
Thanks!
Brian
>
> * @dest: Where to copy to
> * @src: Where to copy from
> * @count: The size of the area.
>> + */
>> +static inline void *memcpy_nocache(void *dest, const void *src, size_t count)
>> +{
>> + return memcpy(dest, src, count);
>> +}
>> +#endif
>
> Thanks,
>
> tglx
Powered by blists - more mailing lists