From: Jisheng Zhang <jszhang@kernel.org> To: Nick Kossifidis <mick@ics.forth.gr> Cc: Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Matteo Croce <mcroce@microsoft.com>, kernel test robot <lkp@intel.com> Subject: Re: [PATCH 2/3] riscv: optimized memmove Date: Wed, 31 Jan 2024 13:25:10 +0800 [thread overview] Message-ID: <ZbnZthTjiUArHIvq@xhacker> (raw) In-Reply-To: <c6066469-7bc9-4232-b600-0b167193f13f@ics.forth.gr> On Tue, Jan 30, 2024 at 06:52:24PM +0200, Nick Kossifidis wrote: > On 1/30/24 15:12, Jisheng Zhang wrote: > > On Tue, Jan 30, 2024 at 01:39:10PM +0200, Nick Kossifidis wrote: > > > On 1/28/24 13:10, Jisheng Zhang wrote: > > > > From: Matteo Croce <mcroce@microsoft.com> > > > > > > > > When the destination buffer is before the source one, or when the > > > > buffers doesn't overlap, it's safe to use memcpy() instead, which is > > > > optimized to use a bigger data size possible. > > > > > > > > Signed-off-by: Matteo Croce <mcroce@microsoft.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > > > > I'd expect to have memmove handle both fw/bw copying and then memcpy being > > > an alias to memmove, to also take care when regions overlap and avoid > > > undefined behavior. > > > > Hi Nick, > > > > Here is somthing from man memcpy: > > > > "void *memcpy(void dest[restrict .n], const void src[restrict .n], > > size_t n); > > > > The memcpy() function copies n bytes from memory area src to memory area dest. > > The memory areas must not overlap. Use memmove(3) if the memory areas do over‐ > > lap." > > > > IMHO, the "restrict" implies that there's no overlap. If overlap > > happens, the manual doesn't say what will happen. > > > > From another side, I have a concern: currently, other arch don't have > > this alias behavior, IIUC(at least, per my understanding of arm and arm64 > > memcpy implementations)they just copy forward. I want to keep similar behavior > > for riscv. > > > > So I want to hear more before going towards alias-memcpy-to-memmove direction. > > > > Thanks > Hi Nick, > If you read Matteo's original post that was also his suggestion, and Linus I did read all discussions in Matteo's v1 ~ v5 before this renew. Per my understanding, Matteo also concerned no such memcpy-alias-memmove behavior in other arch's implementations. > has also commented on that. In general it's better to handle the case where Linus commented on https://bugzilla.redhat.com/show_bug.cgi?id=638477#c132 about glibc alias memcpy to memove rather than the patch series. > the regions provided to memcpy() overlap than to resort to "undefined > behavior", I provided a backwards copy example that you can use so that we > can have both fw and bw copying for memmove(), and use memmove() in any > case. The [restrict .n] in the prototype is just there to say that the size > of src is restricted by n (the next argument). If someone uses memcpy() with I didn't have c99 spec in hand, but I found gcc explanations about restrict keyword from [1]: "the restrict declaration promises that the code will not access that object in any other way--only through p." So if there's overlap in memcpy, then it contradicts the restrict implication. [1] https://www.gnu.org/software/c-intro-and-ref/manual/html_node/restrict-Pointers.html And from the manual, if the memcpy users must ensure "The memory areas must not overlap." So I think all linux kernel's memcpy implementations(only copy fw and don't take overlap into consideration) are right. I did see the alias-memcpy-as-memmove in some libc implementations, but this is not the style in current kernel's implementations. Given current riscv asm implementation also doesn't do the alias and copy-fw only, and this series improves performance and doesn't introduce the Is it better to divide this into two steps: Firstly, merge this series if there's no obvious bug; secondly, do the alias as you suggested, since you have a basic implementation, you could even submit your patch ;) What do you think about this two steps solution? Thanks > overlapping regions, which is always a possibility, in your case it'll > result corrupted data, we won't even get a warning (still counts as > undefined behavior) about it. > > Regards, > Nick >
WARNING: multiple messages have this Message-ID (diff)
From: Jisheng Zhang <jszhang@kernel.org> To: Nick Kossifidis <mick@ics.forth.gr> Cc: Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Matteo Croce <mcroce@microsoft.com>, kernel test robot <lkp@intel.com> Subject: Re: [PATCH 2/3] riscv: optimized memmove Date: Wed, 31 Jan 2024 13:25:10 +0800 [thread overview] Message-ID: <ZbnZthTjiUArHIvq@xhacker> (raw) In-Reply-To: <c6066469-7bc9-4232-b600-0b167193f13f@ics.forth.gr> On Tue, Jan 30, 2024 at 06:52:24PM +0200, Nick Kossifidis wrote: > On 1/30/24 15:12, Jisheng Zhang wrote: > > On Tue, Jan 30, 2024 at 01:39:10PM +0200, Nick Kossifidis wrote: > > > On 1/28/24 13:10, Jisheng Zhang wrote: > > > > From: Matteo Croce <mcroce@microsoft.com> > > > > > > > > When the destination buffer is before the source one, or when the > > > > buffers doesn't overlap, it's safe to use memcpy() instead, which is > > > > optimized to use a bigger data size possible. > > > > > > > > Signed-off-by: Matteo Croce <mcroce@microsoft.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > > > > I'd expect to have memmove handle both fw/bw copying and then memcpy being > > > an alias to memmove, to also take care when regions overlap and avoid > > > undefined behavior. > > > > Hi Nick, > > > > Here is somthing from man memcpy: > > > > "void *memcpy(void dest[restrict .n], const void src[restrict .n], > > size_t n); > > > > The memcpy() function copies n bytes from memory area src to memory area dest. > > The memory areas must not overlap. Use memmove(3) if the memory areas do over‐ > > lap." > > > > IMHO, the "restrict" implies that there's no overlap. If overlap > > happens, the manual doesn't say what will happen. > > > > From another side, I have a concern: currently, other arch don't have > > this alias behavior, IIUC(at least, per my understanding of arm and arm64 > > memcpy implementations)they just copy forward. I want to keep similar behavior > > for riscv. > > > > So I want to hear more before going towards alias-memcpy-to-memmove direction. > > > > Thanks > Hi Nick, > If you read Matteo's original post that was also his suggestion, and Linus I did read all discussions in Matteo's v1 ~ v5 before this renew. Per my understanding, Matteo also concerned no such memcpy-alias-memmove behavior in other arch's implementations. > has also commented on that. In general it's better to handle the case where Linus commented on https://bugzilla.redhat.com/show_bug.cgi?id=638477#c132 about glibc alias memcpy to memove rather than the patch series. > the regions provided to memcpy() overlap than to resort to "undefined > behavior", I provided a backwards copy example that you can use so that we > can have both fw and bw copying for memmove(), and use memmove() in any > case. The [restrict .n] in the prototype is just there to say that the size > of src is restricted by n (the next argument). If someone uses memcpy() with I didn't have c99 spec in hand, but I found gcc explanations about restrict keyword from [1]: "the restrict declaration promises that the code will not access that object in any other way--only through p." So if there's overlap in memcpy, then it contradicts the restrict implication. [1] https://www.gnu.org/software/c-intro-and-ref/manual/html_node/restrict-Pointers.html And from the manual, if the memcpy users must ensure "The memory areas must not overlap." So I think all linux kernel's memcpy implementations(only copy fw and don't take overlap into consideration) are right. I did see the alias-memcpy-as-memmove in some libc implementations, but this is not the style in current kernel's implementations. Given current riscv asm implementation also doesn't do the alias and copy-fw only, and this series improves performance and doesn't introduce the Is it better to divide this into two steps: Firstly, merge this series if there's no obvious bug; secondly, do the alias as you suggested, since you have a basic implementation, you could even submit your patch ;) What do you think about this two steps solution? Thanks > overlapping regions, which is always a possibility, in your case it'll > result corrupted data, we won't even get a warning (still counts as > undefined behavior) about it. > > Regards, > Nick > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2024-01-31 5:38 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-01-28 11:10 [PATCH 0/3] riscv: optimize memcpy/memmove/memset Jisheng Zhang 2024-01-28 11:10 ` Jisheng Zhang 2024-01-28 11:10 ` [PATCH 1/3] riscv: optimized memcpy Jisheng Zhang 2024-01-28 11:10 ` Jisheng Zhang 2024-01-28 12:35 ` David Laight 2024-01-28 12:35 ` David Laight 2024-01-30 12:11 ` Nick Kossifidis 2024-01-30 12:11 ` Nick Kossifidis 2024-01-30 22:44 ` kernel test robot 2024-01-31 0:19 ` kernel test robot 2024-01-31 0:19 ` kernel test robot 2024-01-28 11:10 ` [PATCH 2/3] riscv: optimized memmove Jisheng Zhang 2024-01-28 11:10 ` Jisheng Zhang 2024-01-28 12:47 ` David Laight 2024-01-28 12:47 ` David Laight 2024-01-30 11:30 ` Jisheng Zhang 2024-01-30 11:30 ` Jisheng Zhang 2024-01-30 11:51 ` David Laight 2024-01-30 11:51 ` David Laight 2024-01-30 11:39 ` Nick Kossifidis 2024-01-30 11:39 ` Nick Kossifidis 2024-01-30 13:12 ` Jisheng Zhang 2024-01-30 13:12 ` Jisheng Zhang 2024-01-30 16:52 ` Nick Kossifidis 2024-01-30 16:52 ` Nick Kossifidis 2024-01-31 5:25 ` Jisheng Zhang [this message] 2024-01-31 5:25 ` Jisheng Zhang 2024-01-31 9:13 ` Nick Kossifidis 2024-01-31 9:13 ` Nick Kossifidis 2024-01-28 11:10 ` [PATCH 3/3] riscv: optimized memset Jisheng Zhang 2024-01-28 11:10 ` Jisheng Zhang 2024-01-30 12:07 ` Nick Kossifidis 2024-01-30 12:07 ` Nick Kossifidis 2024-01-30 13:25 ` Jisheng Zhang 2024-01-30 13:25 ` Jisheng Zhang 2024-02-01 23:04 ` David Laight 2024-02-01 23:04 ` David Laight 2024-01-29 18:16 ` [PATCH 0/3] riscv: optimize memcpy/memmove/memset Conor Dooley 2024-01-29 18:16 ` Conor Dooley 2024-01-30 2:28 ` Jisheng Zhang 2024-01-30 2:28 ` Jisheng Zhang -- strict thread matches above, loose matches on Subject: below -- 2021-06-15 2:38 [PATCH 0/3] riscv: optimized mem* functions Matteo Croce 2021-06-15 2:38 ` [PATCH 2/3] riscv: optimized memmove Matteo Croce 2021-06-15 2:38 ` Matteo Croce
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ZbnZthTjiUArHIvq@xhacker \ --to=jszhang@kernel.org \ --cc=aou@eecs.berkeley.edu \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=lkp@intel.com \ --cc=mcroce@microsoft.com \ --cc=mick@ics.forth.gr \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.