Re: [PATCH rdma-core 5/5] Add mmio_memcpy_x64

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Tue, Apr 18, 2017 at 07:22:07PM +0300, Yishai Hadas wrote:
> >@@ -239,19 +239,14 @@ static void set_data_ptr_seg_atomic(struct mlx5_wqe_data_seg *dseg,
> > static void mlx5_bf_copy(unsigned long long *dst, unsigned long long *src,
> > 			 unsigned bytecnt, struct mlx5_qp *qp)
> > {
> >-	while (bytecnt > 0) {
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		*dst++ = *src++;
> >-		bytecnt -= 8 * sizeof(unsigned long long);
> >+	do {
> >+		mmio_memcpy_x64(dst, src, 64);
> >+		bytecnt -= 64;
> >+		dst += 8;
> >+		src += 8;
> 
> It looks like the above +=8 is wrong in 32 bit systems, agree ?

Hurm. On 32 bit systems 'unsigned long long' will still be 64 bit, so
the above is OK.

The above original is buggy on 32 bit because it is not guarenteed to
generate stores strictly in increasing address order. I think the
author's intent was to have used 'uintptr_t *'.

I will change the arguments to be 'uint64_t *' for clarity.

> >+	/* Use the native word size for the copy */
> >+	if (sizeof(*dst_p) == 8) {
> 
> We expect this 'if' to be dropped at compile time to prevent performance
> penalty comparing the original code, correct ?

Yes.

The entire mmio_memcpy_x64 expands to a bunch of movs with no branches
as the transfer size is constant as well.

The overall mlx5_bf_copy looses one branch because of the
transformation to do/while

> >+		} while (bytecnt > 0);
> >+	} else if (sizeof(*dst_p) == 4) {
> >+		const __be32 *src_p = src;
> >+
> >+		do {
> >+			mmio_write32_be(dst_p++, *src_p++);
> >+			mmio_write32_be(dst_p++, *src_p++);
> >+			bytecnt -= 2 * sizeof(*dst_p);
> 
> Any reason not to write at least 64 bytes here before checking byte count
> and looping again ?

icache size? I debated doing that, but the consensus of the existing
implementations seems to be against it..

We could do a 32 byte unwind which is probably a similar icache
footprint?

What would you like?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html