From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970551AbdAELkW (ORCPT ); Thu, 5 Jan 2017 06:40:22 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:40231 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S970522AbdAELkP (ORCPT ); Thu, 5 Jan 2017 06:40:15 -0500 Date: Thu, 5 Jan 2017 14:39:55 +0300 From: Dan Carpenter To: Long Li Cc: "K. Y. Srinivasan" , Haiyang Zhang , devel@linuxdriverproject.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] hv: use substraction to update ring buffer index Message-ID: <20170105113955.GD13756@mwanda> References: <1483589302-27177-1-git-send-email-longli@exchange.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1483589302-27177-1-git-send-email-longli@exchange.microsoft.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 04, 2017 at 08:08:22PM -0800, Long Li wrote: > From: Long Li > > 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 > --- > 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