* [PATCH] hv: use substraction to update ring buffer index
@ 2017-01-05 4:08 Long Li
2017-01-05 11:39 ` Dan Carpenter
2017-01-16 3:12 ` Dexuan Cui
0 siblings, 2 replies; 6+ messages in thread
From: Long Li @ 2017-01-05 4:08 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang; +Cc: devel, linux-kernel, Long Li
From: Long Li <longli@microsoft.com>
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 <longli@microsoft.com>
---
drivers/hv/ring_buffer.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index cd49cb1..f8eee6e 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -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;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hv: use substraction to update ring buffer index
2017-01-05 4:08 [PATCH] hv: use substraction to update ring buffer index Long Li
@ 2017-01-05 11:39 ` Dan Carpenter
2017-01-05 11:48 ` Dan Carpenter
2017-01-07 7:15 ` Long Li
2017-01-16 3:12 ` Dexuan Cui
1 sibling, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-01-05 11:39 UTC (permalink / raw)
To: Long Li; +Cc: K. Y. Srinivasan, Haiyang Zhang, devel, linux-kernel
On Wed, Jan 04, 2017 at 08:08:22PM -0800, Long Li wrote:
> From: Long Li <longli@microsoft.com>
>
> 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 <longli@microsoft.com>
> ---
> drivers/hv/ring_buffer.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index cd49cb1..f8eee6e 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -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...
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hv: use substraction to update ring buffer index
2017-01-05 11:39 ` Dan Carpenter
@ 2017-01-05 11:48 ` Dan Carpenter
2017-01-07 7:15 ` Long Li
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-01-05 11:48 UTC (permalink / raw)
To: Long Li; +Cc: devel, Haiyang Zhang, linux-kernel
On Thu, Jan 05, 2017 at 02:39:55PM +0300, Dan Carpenter wrote:
> > @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] hv: use substraction to update ring buffer index
2017-01-05 11:39 ` Dan Carpenter
2017-01-05 11:48 ` Dan Carpenter
@ 2017-01-07 7:15 ` Long Li
1 sibling, 0 replies; 6+ messages in thread
From: Long Li @ 2017-01-07 7:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: KY Srinivasan, Haiyang Zhang, devel, linux-kernel
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, January 05, 2017 3:40 AM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] hv: use substraction to update ring buffer index
>
> On Wed, Jan 04, 2017 at 08:08:22PM -0800, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > 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 <longli@microsoft.com>
> > ---
> > drivers/hv/ring_buffer.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c index
> > cd49cb1..f8eee6e 100644
> > --- a/drivers/hv/ring_buffer.c
> > +++ b/drivers/hv/ring_buffer.c
> > @@ -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...
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.
>
> >
> > 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().
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.
>
> regards,
> dan carpenter
>
Thanks for reviewing!
Long
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] hv: use substraction to update ring buffer index
2017-01-05 4:08 [PATCH] hv: use substraction to update ring buffer index Long Li
2017-01-05 11:39 ` Dan Carpenter
@ 2017-01-16 3:12 ` Dexuan Cui
2017-01-20 21:15 ` Long Li
1 sibling, 1 reply; 6+ messages in thread
From: Dexuan Cui @ 2017-01-16 3:12 UTC (permalink / raw)
To: Long Li, KY Srinivasan, Haiyang Zhang; +Cc: devel, linux-kernel
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of Long Li
> Sent: Thursday, January 5, 2017 12:08
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] hv: use substraction to update ring buffer index
>
> From: Long Li <longli@microsoft.com>
>
> 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 <longli@microsoft.com>
> ---
> drivers/hv/ring_buffer.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index cd49cb1..f8eee6e 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -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;
> }
Hi Long,
I guess you want to fix put_pkt_raw() too. :-)
Thanks,
-- Dexuan
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] hv: use substraction to update ring buffer index
2017-01-16 3:12 ` Dexuan Cui
@ 2017-01-20 21:15 ` Long Li
0 siblings, 0 replies; 6+ messages in thread
From: Long Li @ 2017-01-20 21:15 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang; +Cc: devel, linux-kernel
> -----Original Message-----
> From: Dexuan Cui
> Sent: Sunday, January 15, 2017 7:12 PM
> To: Long Li <longli@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>
> Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] hv: use substraction to update ring buffer index
>
> > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> > Behalf Of Long Li
> > Sent: Thursday, January 5, 2017 12:08
> > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>
> > Cc: devel@linuxdriverproject.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH] hv: use substraction to update ring buffer index
> >
> > From: Long Li <longli@microsoft.com>
> >
> > 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 <longli@microsoft.com>
> > ---
> > drivers/hv/ring_buffer.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c index
> > cd49cb1..f8eee6e 100644
> > --- a/drivers/hv/ring_buffer.c
> > +++ b/drivers/hv/ring_buffer.c
> > @@ -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;
> > }
>
> Hi Long,
> I guess you want to fix put_pkt_raw() too. :-)
Good point. I will send an updated patch.
>
> Thanks,
> -- Dexuan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-20 21:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-05 4:08 [PATCH] hv: use substraction to update ring buffer index Long Li
2017-01-05 11:39 ` Dan Carpenter
2017-01-05 11:48 ` Dan Carpenter
2017-01-07 7:15 ` Long Li
2017-01-16 3:12 ` Dexuan Cui
2017-01-20 21:15 ` Long Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).