hv: use substraction to update ring buffer index

PATCH In review Long Li 4 participants 6 messages Jan 5, 2017 – Jan 20, 2017 Hyper-V/Azure core and drivers

lore

Share & tools

lore

Long Li

author

2017-01-05 02:12 UTC #0 lore raw

PATCH 1/1 hv: use substraction to update ring buffer index

From: Long Li <[email protected]>

The ring buffer code uses %= to calculate index. For x86/64, %= compiles to div, more than 10 times slower than sub.

Replace div with sub for this data heavy code path.

Signed-off-by Long Li <[email protected]>

---
 drivers/hv/ring_buffer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/hv/ring_buffer.c +6 -3

index cd49cb1..f8eee6e 100644
@@ -135,7 +135,8 @@ hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info,
 	u32 next = ring_info->ring_buffer->read_index;
 
 	next += offset;
-	next %= ring_info->ring_datasize;
+	if (next >= ring_info->ring_datasize)
+		next -= ring_info->ring_datasize;
 
 	return next;
 }

@@ -179,7 +180,8 @@ static u32 hv_copyfrom_ringbuffer(
 	memcpy(dest, ring_buffer + start_read_offset, destlen);
 
 	start_read_offset += destlen;
-	start_read_offset %= ring_buffer_size;
+	if (start_read_offset >= ring_buffer_size)
+		start_read_offset -= ring_buffer_size;
 
 	return start_read_offset;
 }

@@ -201,7 +203,8 @@ static u32 hv_copyto_ringbuffer(
 	memcpy(ring_buffer + start_write_offset, src, srclen);
 
 	start_write_offset += srclen;
-	start_write_offset %= ring_buffer_size;
+	if (start_write_offset >= ring_buffer_size)
+		start_write_offset -= ring_buffer_size;
 
 	return start_write_offset;
 }

Signature · 1 line

2.7.4

Dan Carpenter

2017-01-05 11:40 UTC #1 lore raw

Quoted 17 lines · Long Li

From: Long Li <[email protected]>
The ring buffer code uses %= to calculate index. For x86/64, %= compiles to div, more than 10 times slower than sub.
Replace div with sub for this data heavy code path.
Signed-off-by Long Li <[email protected]>
---
 drivers/hv/ring_buffer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/hv/ring_buffer.c +0 -0
index cd49cb1..f8eee6e 100644

drivers/hv/ring_buffer.c hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info, · line 139

Quoted 6 lines

drivers/hv/ring_buffer.c +2 -1

@@ -135,7 +135,8 @@ hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info,
 	u32 next = ring_info->ring_buffer->read_index;
 
 	next += offset;
-	next %= ring_info->ring_datasize;
+	if (next >= ring_info->ring_datasize)
+		next -= ring_info->ring_datasize;

I take it that we trust that offset is roughly correct and not more than 2x ring_info->ring_datasize? I guess there is only one caller so it's probably true...

drivers/hv/ring_buffer.c static u32 hv_copyfrom_ringbuffer( · line 184

Quoted 10 lines

drivers/hv/ring_buffer.c +2 -1

 	return next;
 }

@@ -179,7 +180,8 @@ static u32 hv_copyfrom_ringbuffer(
 	memcpy(dest, ring_buffer + start_read_offset, destlen);
 
 	start_read_offset += destlen;
-	start_read_offset %= ring_buffer_size;
+	if (start_read_offset >= ring_buffer_size)
+		start_read_offset -= ring_buffer_size;

I totally don't understand the original code here. We do the memset and then we verify that we are not copying beyond the end of the ring buffer? If feels like we should verify that offset + destlen aren't more than ring_buffer_size before we do the memcpy().

regards,
dan carpenter

Dan Carpenter

2017-01-05 11:51 UTC #2 lore raw

Quoted 9 lines · Dan Carpenter

drivers/hv/ring_buffer.c static u32 hv_copyfrom_ringbuffer( · line 184
Quoted 7 lines
drivers/hv/ring_buffer.c +2 -1
@@ -179,7 +180,8 @@ static u32 hv_copyfrom_ringbuffer(
 	memcpy(dest, ring_buffer + start_read_offset, destlen);
 
 	start_read_offset += destlen;
-	start_read_offset %= ring_buffer_size;
+	if (start_read_offset >= ring_buffer_size)
+		start_read_offset -= ring_buffer_size;
I totally don't understand the original code here. We do the memset

I meant memcpy() not memset.

regards,
dan carpenter

Long Li

2017-01-07 07:15 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 32 lines

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, January 05, 2017 3:40 AM
To: Long Li <[email protected]>
Cc: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; [email protected]; linux- [email protected]
Subject: Re: [PATCH] hv: use substraction to update ring buffer index
drivers/hv/ring_buffer.c hv_get_next_readlocation_withoffset(struct
Quoted 17 lines · Long Li
From: Long Li <[email protected]>
The ring buffer code uses %= to calculate index. For x86/64, %= compiles to div, more than 10 times slower than sub.
Replace div with sub for this data heavy code path.
Signed-off-by Long Li <[email protected]>
---
 drivers/hv/ring_buffer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/hv/ring_buffer.c index renamed from drivers/hv/ring_buffer.c +0 -0
cd49cb1..f8eee6e 100644
drivers/hv/ring_buffer.c +0 -0
@@ -135,7 +135,8 @@ hv_get_next_readlocation_withoffset(struct
hv_ring_buffer_info *ring_info,
Quoted 2 lines
drivers/hv/ring_buffer.c +0 -0
 	u32 next = ring_info->ring_buffer->read_index;
 

Quoted 8 lines

drivers/hv/ring_buffer.c hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info, · line 139
Quoted 4 lines
drivers/hv/ring_buffer.c +2 -1
 	next += offset;
-	next %= ring_info->ring_datasize;
+	if (next >= ring_info->ring_datasize)
+		next -= ring_info->ring_datasize;
I take it that we trust that offset is roughly correct and not more than 2x ring_info->ring_datasize? I guess there is only one caller so it's probably true...

Yes, you are right. It's not possible that we are getting to 2x ring_datasize, because it's not possible to transfer data more than ring_datasize over ring buffer.

Quoted 7 lines

drivers/hv/ring_buffer.c +0 -0

 	return next;
 }

@@ -179,7 +180,8 @@ static u32 hv_copyfrom_ringbuffer(
 	memcpy(dest, ring_buffer + start_read_offset, destlen);

Quoted 9 lines

drivers/hv/ring_buffer.c static u32 hv_copyfrom_ringbuffer( · line 184
Quoted 4 lines
drivers/hv/ring_buffer.c +2 -1
 	start_read_offset += destlen;
-	start_read_offset %= ring_buffer_size;
+	if (start_read_offset >= ring_buffer_size)
+		start_read_offset -= ring_buffer_size;
I totally don't understand the original code here. We do the memset and then we verify that we are not copying beyond the end of the ring buffer? If feels like we should verify that offset + destlen aren't more than ring_buffer_size before we do the memcpy().

The ring buffer pages are mapped to wraparound 2x virtual address space. Please see hv_ringbuffer_init(). The call to vmap() setup this virtual address space. So we can use memcpy across the last page.

Quoted 4 lines

regards,
dan carpenter

Thanks for reviewing!

Long

Dexuan Cui

maintainer

2017-01-16 03:12 UTC #4 lore raw

In reply to Long Li

Quoted 46 lines

From: devel [mailto:[email protected]] On Behalf Of Long Li
Sent: Thursday, January 5, 2017 12:08
To: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>
Cc: [email protected]; [email protected]
Subject: [PATCH] hv: use substraction to update ring buffer index
From: Long Li <[email protected]>
The ring buffer code uses %= to calculate index. For x86/64, %= compiles to div, more than 10 times slower than sub.
Replace div with sub for this data heavy code path.
Signed-off-by Long Li <[email protected]>
---
 drivers/hv/ring_buffer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/hv/ring_buffer.c +2 -1
index cd49cb1..f8eee6e 100644
@@ -135,7 +135,8 @@ hv_get_next_readlocation_withoffset(struct
 hv_ring_buffer_info *ring_info,
        u32 next = ring_info->ring_buffer->read_index;
 
        next += offset;
-       next %= ring_info->ring_datasize;
+       if (next >= ring_info->ring_datasize)
+               next -= ring_info->ring_datasize;
 
        return next;
}
drivers/hv/ring_buffer.c +2 -1
@@ -179,7 +180,8 @@ static u32 hv_copyfrom_ringbuffer(
        memcpy(dest, ring_buffer + start_read_offset, destlen);
 
        start_read_offset += destlen;
-       start_read_offset %= ring_buffer_size;
+       if (start_read_offset >= ring_buffer_size)
+               start_read_offset -= ring_buffer_size;
 
        return start_read_offset;
 }

drivers/hv/ring_buffer.c static u32 hv_copyto_ringbuffer( · line 210

Quoted 9 lines

drivers/hv/ring_buffer.c +2 -1

@@ -201,7 +203,8 @@ static u32 hv_copyto_ringbuffer(
        memcpy(ring_buffer + start_write_offset, src, srclen);
 
        start_write_offset += srclen;
-       start_write_offset %= ring_buffer_size;
+       if (start_write_offset >= ring_buffer_size)
+               start_write_offset -= ring_buffer_size;
 
        return start_write_offset;
 }

Hi Long,
I guess you want to fix put_pkt_raw() too. :-)

Thanks,
-- Dexuan

Long Li

2017-01-20 21:15 UTC #5 lore raw

Quoted 63 lines

-----Original Message-----
From: Dexuan Cui
Sent: Sunday, January 15, 2017 7:12 PM
To: Long Li <[email protected]>; KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>
Cc: [email protected]; [email protected]
Subject: RE: [PATCH] hv: use substraction to update ring buffer index
Quoted 53 lines
From: devel [mailto:[email protected]] On Behalf Of Long Li
Sent: Thursday, January 5, 2017 12:08
To: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>
Cc: [email protected]; [email protected]
Subject: [PATCH] hv: use substraction to update ring buffer index
From: Long Li <[email protected]>
The ring buffer code uses %= to calculate index. For x86/64, %= compiles to div, more than 10 times slower than sub.
Replace div with sub for this data heavy code path.
Signed-off-by Long Li <[email protected]>
---
 drivers/hv/ring_buffer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/hv/ring_buffer.c index renamed from drivers/hv/ring_buffer.c +0 -0
cd49cb1..f8eee6e 100644
drivers/hv/ring_buffer.c +2 -1
@@ -135,7 +135,8 @@ hv_get_next_readlocation_withoffset(struct
 hv_ring_buffer_info *ring_info,
        u32 next = ring_info->ring_buffer->read_index;
 
        next += offset;
-       next %= ring_info->ring_datasize;
+       if (next >= ring_info->ring_datasize)
+               next -= ring_info->ring_datasize;
 
        return next;
}
drivers/hv/ring_buffer.c +4 -2
@@ -179,7 +180,8 @@ static u32 hv_copyfrom_ringbuffer(
        memcpy(dest, ring_buffer + start_read_offset, destlen);
 
        start_read_offset += destlen;
-       start_read_offset %= ring_buffer_size;
+       if (start_read_offset >= ring_buffer_size)
+               start_read_offset -= ring_buffer_size;
 
        return start_read_offset;
 }
@@ -201,7 +203,8 @@ static u32 hv_copyto_ringbuffer(
        memcpy(ring_buffer + start_write_offset, src, srclen);
 
        start_write_offset += srclen;
-       start_write_offset %= ring_buffer_size;
+       if (start_write_offset >= ring_buffer_size)
+               start_write_offset -= ring_buffer_size;
 

Quoted 5 lines

drivers/hv/ring_buffer.c static u32 hv_copyto_ringbuffer( · line 210
Quoted 2 lines
drivers/hv/ring_buffer.c +0 -0
        return start_write_offset;
 }
Hi Long,
I guess you want to fix put_pkt_raw() too. :-)

Good point. I will send an updated patch.

Quoted 3 lines

Thanks,
-- Dexuan