| Message ID | 1449199149-5078-2-git-send-email-seyeong.kim@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: > From: Haiyang Zhang <haiyangz@microsoft.com> > > BugLink: http://bugs.launchpad.net/bugs/1521053 > > The commit > > hv_netvsc: Clean up two unused variables As this commit only removes unused variables, it is not at all clear how this could cause or fix network slowdowns. The net effect is to remove 64 bytes from one structure, but this seems unlikely enough to cause significant performance issues. How was it determined that this was the fix? -apw > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > Reviewed-by: K. Y. Srinivasan <kys@microsoft.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (backported from commit 0d158852a8089099a6959ae235b20f230871982f) > Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com> > --- > drivers/net/hyperv/hyperv_net.h | 1 - > drivers/net/hyperv/netvsc.c | 1 - > drivers/net/hyperv/rndis_filter.c | 2 -- > 3 files changed, 4 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index bf2604b..ce9d49b 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -129,7 +129,6 @@ struct hv_netvsc_packet { > /* Bookkeeping stuff */ > u32 status; > > - struct hv_device *device; > bool is_data_pkt; > bool xmit_more; /* from skb */ > bool cp_partial; /* partial copy into send buffer */ > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index b15041b..9e4a86f 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -1037,7 +1037,6 @@ static void netvsc_receive(struct netvsc_device *net_device, > } > > count = vmxferpage_packet->range_cnt; > - netvsc_packet->device = device; > netvsc_packet->channel = channel; > > /* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */ > diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c > index 524cb82..11598f0 100644 > --- a/drivers/net/hyperv/rndis_filter.c > +++ b/drivers/net/hyperv/rndis_filter.c > @@ -47,8 +47,6 @@ struct rndis_request { > > /* Simplify allocation by having a netvsc packet inline */ > struct hv_netvsc_packet pkt; > - /* Set 2 pages for rndis requests crossing page boundary */ > - struct hv_page_buffer buf[2]; > > struct rndis_message request_msg; > /* > -- > 2.1.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 12/04/2015 05:02 AM, Andy Whitcroft wrote: > On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: >> From: Haiyang Zhang <haiyangz@microsoft.com> >> >> BugLink: http://bugs.launchpad.net/bugs/1521053 >> >> The commit >> >> hv_netvsc: Clean up two unused variables > > As this commit only removes unused variables, it is not at all clear how > this could cause or fix network slowdowns. The net effect is to remove > 64 bytes from one structure, but this seems unlikely enough to cause > significant performance issues. > > How was it determined that this was the fix? > > -apw I'm with Andy on this one. How does this really fix the problem ? rtg
On Fri, Dec 04, 2015 at 06:56:08AM -0700, Tim Gardner wrote: > On 12/04/2015 05:02 AM, Andy Whitcroft wrote: > > On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: > >> From: Haiyang Zhang <haiyangz@microsoft.com> > >> > >> BugLink: http://bugs.launchpad.net/bugs/1521053 > >> > >> The commit > >> > >> hv_netvsc: Clean up two unused variables > > > > As this commit only removes unused variables, it is not at all clear how > > this could cause or fix network slowdowns. The net effect is to remove > > 64 bytes from one structure, but this seems unlikely enough to cause > > significant performance issues. > > > > How was it determined that this was the fix? > > > > -apw > > I'm with Andy on this one. How does this really fix the problem ? It is sounding like this does actually fix something. For that to be true this layout change would need to radically affect cache performance. But if the testing can be confirmed then it has my ack. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
On 12/07/2015 10:07 AM, Andy Whitcroft wrote: > On Fri, Dec 04, 2015 at 06:56:08AM -0700, Tim Gardner wrote: >> On 12/04/2015 05:02 AM, Andy Whitcroft wrote: >>> On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: >>>> From: Haiyang Zhang <haiyangz@microsoft.com> >>>> >>>> BugLink: http://bugs.launchpad.net/bugs/1521053 >>>> >>>> The commit >>>> >>>> hv_netvsc: Clean up two unused variables >>> >>> As this commit only removes unused variables, it is not at all clear how >>> this could cause or fix network slowdowns. The net effect is to remove >>> 64 bytes from one structure, but this seems unlikely enough to cause >>> significant performance issues. >>> >>> How was it determined that this was the fix? >>> >>> -apw >> >> I'm with Andy on this one. How does this really fix the problem ? > > It is sounding like this does actually fix something. For that to be > true this layout change would need to radically affect cache > performance. But if the testing can be confirmed then it has my ack. > > Acked-by: Andy Whitcroft <apw@canonical.com> > > -apw > Given the carnage required to revert 'hv_netvsc: Implement partial copy into send buffer' I'm inclined to ACK this as well. It is a harmless fix and appears to have a real impact on performance.
On Mon, Dec 07, 2015 at 05:07:35PM +0000, Andy Whitcroft wrote: > On Fri, Dec 04, 2015 at 06:56:08AM -0700, Tim Gardner wrote: > > On 12/04/2015 05:02 AM, Andy Whitcroft wrote: > > > On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: > > >> From: Haiyang Zhang <haiyangz@microsoft.com> > > >> > > >> BugLink: http://bugs.launchpad.net/bugs/1521053 > > >> > > >> The commit > > >> > > >> hv_netvsc: Clean up two unused variables > > > > > > As this commit only removes unused variables, it is not at all clear how > > > this could cause or fix network slowdowns. The net effect is to remove > > > 64 bytes from one structure, but this seems unlikely enough to cause > > > significant performance issues. > > > > > > How was it determined that this was the fix? > > > > > > -apw > > > > I'm with Andy on this one. How does this really fix the problem ? > > It is sounding like this does actually fix something. For that to be > true this layout change would need to radically affect cache > performance. But if the testing can be confirmed then it has my ack. > > Acked-by: Andy Whitcroft <apw@canonical.com> Confirmed that removing these four bytes reduce the header size such that it fits into the standard packet headroom and we no longer have to copy the packet headers every time. -apw
Andy Whitcroft <apw@canonical.com> wrote: >On Mon, Dec 07, 2015 at 05:07:35PM +0000, Andy Whitcroft wrote: >> On Fri, Dec 04, 2015 at 06:56:08AM -0700, Tim Gardner wrote: >> > On 12/04/2015 05:02 AM, Andy Whitcroft wrote: >> > > On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: >> > >> From: Haiyang Zhang <haiyangz@microsoft.com> >> > >> >> > >> BugLink: http://bugs.launchpad.net/bugs/1521053 >> > >> >> > >> The commit >> > >> >> > >> hv_netvsc: Clean up two unused variables >> > > >> > > As this commit only removes unused variables, it is not at all clear how >> > > this could cause or fix network slowdowns. The net effect is to remove >> > > 64 bytes from one structure, but this seems unlikely enough to cause >> > > significant performance issues. >> > > >> > > How was it determined that this was the fix? >> > > >> > > -apw >> > >> > I'm with Andy on this one. How does this really fix the problem ? >> >> It is sounding like this does actually fix something. For that to be >> true this layout change would need to radically affect cache >> performance. But if the testing can be confirmed then it has my ack. >> >> Acked-by: Andy Whitcroft <apw@canonical.com> > >Confirmed that removing these four bytes reduce the header size such >that it fits into the standard packet headroom and we no longer have to >copy the packet headers every time. We also would like to request that this patch be applied as an SRU for the Wily kernel as well to avoid regression during upgrades. -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Thu, Dec 10, 2015 at 11:08:22AM -0800, Jay Vosburgh wrote: > Andy Whitcroft <apw@canonical.com> wrote: > > >On Mon, Dec 07, 2015 at 05:07:35PM +0000, Andy Whitcroft wrote: > >> On Fri, Dec 04, 2015 at 06:56:08AM -0700, Tim Gardner wrote: > >> > On 12/04/2015 05:02 AM, Andy Whitcroft wrote: > >> > > On Fri, Dec 04, 2015 at 12:19:09PM +0900, Seyeong Kim wrote: > >> > >> From: Haiyang Zhang <haiyangz@microsoft.com> > >> > >> > >> > >> BugLink: http://bugs.launchpad.net/bugs/1521053 > >> > >> > >> > >> The commit > >> > >> > >> > >> hv_netvsc: Clean up two unused variables > >> > > > >> > > As this commit only removes unused variables, it is not at all clear how > >> > > this could cause or fix network slowdowns. The net effect is to remove > >> > > 64 bytes from one structure, but this seems unlikely enough to cause > >> > > significant performance issues. > >> > > > >> > > How was it determined that this was the fix? > >> > > > >> > > -apw > >> > > >> > I'm with Andy on this one. How does this really fix the problem ? > >> > >> It is sounding like this does actually fix something. For that to be > >> true this layout change would need to radically affect cache > >> performance. But if the testing can be confirmed then it has my ack. > >> > >> Acked-by: Andy Whitcroft <apw@canonical.com> > > > >Confirmed that removing these four bytes reduce the header size such > >that it fits into the standard packet headroom and we no longer have to > >copy the packet headers every time. > > We also would like to request that this patch be applied as an > SRU for the Wily kernel as well to avoid regression during upgrades. > > -J Wily already contains this commit. > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Brad
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index bf2604b..ce9d49b 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -129,7 +129,6 @@ struct hv_netvsc_packet { /* Bookkeeping stuff */ u32 status; - struct hv_device *device; bool is_data_pkt; bool xmit_more; /* from skb */ bool cp_partial; /* partial copy into send buffer */ diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index b15041b..9e4a86f 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -1037,7 +1037,6 @@ static void netvsc_receive(struct netvsc_device *net_device, } count = vmxferpage_packet->range_cnt; - netvsc_packet->device = device; netvsc_packet->channel = channel; /* Each range represents 1 RNDIS pkt that contains 1 ethernet frame */ diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index 524cb82..11598f0 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -47,8 +47,6 @@ struct rndis_request { /* Simplify allocation by having a netvsc packet inline */ struct hv_netvsc_packet pkt; - /* Set 2 pages for rndis requests crossing page boundary */ - struct hv_page_buffer buf[2]; struct rndis_message request_msg; /*