Re: [PATCH 1/2 net-next] Drivers: hv: vmbus: Export the vmbus_sendpacket_pagebuffer_ctl()

Re: [PATCH 1/2 net-next] Drivers: hv: vmbus: Export the vmbus_sendpacket_pagebuffer_ctl()

[Greg KH]

On Tue, Mar 10, 2015 at 11:50:03AM -0700, K. Y. Srinivasan wrote:
> Export the vmbus_sendpacket_pagebuffer_ctl() interface. This interface
> will be used in the netvsc driver to optimize signalling the host.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 0/2 net-next] hyperv: Enable batched notification

[K. Y. Srinivasan]

Take into consideration the xmit_more flag in skb to decide if we should
notify the host as we place packets in VMBUS.

The VMBUS API that would give us this control is already in Greg's tree, in this
patch-set, that API is exported so it can be used in the netvsc driver.

K. Y. Srinivasan (2):
  Drivers: hv: vmbus: Export the vmbus_sendpacket_pagebuffer_ctl()
  hyperv: Support batched notification

 drivers/hv/channel.c              |    1 +
 drivers/net/hyperv/hyperv_net.h   |    2 +-
 drivers/net/hyperv/netvsc.c       |   14 +++++++++-----
 drivers/net/hyperv/netvsc_drv.c   |    3 ++-
 drivers/net/hyperv/rndis_filter.c |    2 +-
 5 files changed, 14 insertions(+), 8 deletions(-)

-- 
1.7.4.1

[PATCH 1/2 net-next] Drivers: hv: vmbus: Export the vmbus_sendpacket_pagebuffer_ctl()

[K. Y. Srinivasan]

Export the vmbus_sendpacket_pagebuffer_ctl() interface. This interface
will be used in the netvsc driver to optimize signalling the host.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/channel.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index da53180..e58cdb7 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -710,6 +710,7 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(vmbus_sendpacket_pagebuffer_ctl);
 
 /*
  * vmbus_sendpacket_pagebuffer - Send a range of single-page buffer
-- 
1.7.4.1

[PATCH 2/2 net-next] hyperv: Support batched notification

[K. Y. Srinivasan]

Optimize notifying the host by deferring notification until there
are no more packets to be sent. This will help in batching the requests
on the host.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/net/hyperv/hyperv_net.h   |    2 +-
 drivers/net/hyperv/netvsc.c       |   14 +++++++++-----
 drivers/net/hyperv/netvsc_drv.c   |    3 ++-
 drivers/net/hyperv/rndis_filter.c |    2 +-
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 4815843..3fd9896 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -184,7 +184,7 @@ struct rndis_device {
 int netvsc_device_add(struct hv_device *device, void *additional_info);
 int netvsc_device_remove(struct hv_device *device);
 int netvsc_send(struct hv_device *device,
-		struct hv_netvsc_packet *packet);
+		struct hv_netvsc_packet *packet, bool kick_q);
 void netvsc_linkstatus_callback(struct hv_device *device_obj,
 				struct rndis_message *resp);
 int netvsc_recv_callback(struct hv_device *device_obj,
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 208eb05..9003b94 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -707,7 +707,7 @@ static u32 netvsc_copy_to_send_buf(struct netvsc_device *net_device,
 }
 
 int netvsc_send(struct hv_device *device,
-			struct hv_netvsc_packet *packet)
+			struct hv_netvsc_packet *packet, bool kick_q)
 {
 	struct netvsc_device *net_device;
 	int ret = 0;
@@ -719,6 +719,7 @@ int netvsc_send(struct hv_device *device,
 	u32 msg_size = 0;
 	struct sk_buff *skb = NULL;
 	u16 q_idx = packet->q_idx;
+	u32 vmbus_flags = VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED;
 
 
 	net_device = get_outbound_net_device(device);
@@ -768,18 +769,21 @@ int netvsc_send(struct hv_device *device,
 		return -ENODEV;
 
 	if (packet->page_buf_cnt) {
-		ret = vmbus_sendpacket_pagebuffer(out_channel,
+		ret = vmbus_sendpacket_pagebuffer_ctl(out_channel,
 						  packet->page_buf,
 						  packet->page_buf_cnt,
 						  &sendMessage,
 						  sizeof(struct nvsp_message),
-						  req_id);
+						  req_id,
+						  vmbus_flags,
+						  kick_q);
 	} else {
-		ret = vmbus_sendpacket(out_channel, &sendMessage,
+		ret = vmbus_sendpacket_ctl(out_channel, &sendMessage,
 				sizeof(struct nvsp_message),
 				req_id,
 				VM_PKT_DATA_INBAND,
-				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+				vmbus_flags,
+				kick_q);
 	}
 
 	if (ret == 0) {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index a06bd66..80b4b29 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -384,6 +384,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
 	u32 net_trans_info;
 	u32 hash;
 	u32 skb_length = skb->len;
+	bool kick_q = !skb->xmit_more;
 
 
 	/* We will atmost need two pages to describe the rndis
@@ -556,7 +557,7 @@ do_send:
 	packet->page_buf_cnt = init_page_array(rndis_msg, rndis_msg_size,
 					skb, &packet->page_buf[0]);
 
-	ret = netvsc_send(net_device_ctx->device_ctx, packet);
+	ret = netvsc_send(net_device_ctx->device_ctx, packet, kick_q);
 
 drop:
 	if (ret == 0) {
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index ca81de0..05f3792 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -238,7 +238,7 @@ static int rndis_filter_send_request(struct rndis_device *dev,
 
 	packet->send_completion = NULL;
 
-	ret = netvsc_send(dev->net_dev->dev, packet);
+	ret = netvsc_send(dev->net_dev->dev, packet, true);
 	return ret;
 }
 
-- 
1.7.4.1

Re: [PATCH 2/2 net-next] hyperv: Support batched notification

[Jason Wang]

On Wed, Mar 11, 2015 at 2:50 AM, K. Y. Srinivasan <kys@microsoft.com> 
wrote:
> Optimize notifying the host by deferring notification until there
> are no more packets to be sent. This will help in batching the 
> requests
> on the host.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/net/hyperv/hyperv_net.h   |    2 +-
>  drivers/net/hyperv/netvsc.c       |   14 +++++++++-----
>  drivers/net/hyperv/netvsc_drv.c   |    3 ++-
>  drivers/net/hyperv/rndis_filter.c |    2 +-
>  4 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/hyperv/hyperv_net.h 
> b/drivers/net/hyperv/hyperv_net.h
> index 4815843..3fd9896 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -184,7 +184,7 @@ struct rndis_device {
>  int netvsc_device_add(struct hv_device *device, void 
> *additional_info);
>  int netvsc_device_remove(struct hv_device *device);
>  int netvsc_send(struct hv_device *device,
> -		struct hv_netvsc_packet *packet);
> +		struct hv_netvsc_packet *packet, bool kick_q);
>  void netvsc_linkstatus_callback(struct hv_device *device_obj,
>  				struct rndis_message *resp);
>  int netvsc_recv_callback(struct hv_device *device_obj,
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 208eb05..9003b94 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -707,7 +707,7 @@ static u32 netvsc_copy_to_send_buf(struct 
> netvsc_device *net_device,
>  }
>  
>  int netvsc_send(struct hv_device *device,
> -			struct hv_netvsc_packet *packet)
> +			struct hv_netvsc_packet *packet, bool kick_q)
>  {
>  	struct netvsc_device *net_device;
>  	int ret = 0;
> @@ -719,6 +719,7 @@ int netvsc_send(struct hv_device *device,
>  	u32 msg_size = 0;
>  	struct sk_buff *skb = NULL;
>  	u16 q_idx = packet->q_idx;
> +	u32 vmbus_flags = VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED;
>  
>  
>  	net_device = get_outbound_net_device(device);
> @@ -768,18 +769,21 @@ int netvsc_send(struct hv_device *device,
>  		return -ENODEV;
>  
>  	if (packet->page_buf_cnt) {
> -		ret = vmbus_sendpacket_pagebuffer(out_channel,
> +		ret = vmbus_sendpacket_pagebuffer_ctl(out_channel,
>  						  packet->page_buf,
>  						  packet->page_buf_cnt,
>  						  &sendMessage,
>  						  sizeof(struct nvsp_message),
> -						  req_id);
> +						  req_id,
> +						  vmbus_flags,
> +						  kick_q);

What if kick_q is false but ret is -EAGAIN here? Looks like in this 
case host won't get notified at all. How about checking whether txq and 
kicking if it has been stopped like what other network driver did?
> 
>  	} else {
> -		ret = vmbus_sendpacket(out_channel, &sendMessage,
> +		ret = vmbus_sendpacket_ctl(out_channel, &sendMessage,
>  				sizeof(struct nvsp_message),
>  				req_id,
>  				VM_PKT_DATA_INBAND,
> -				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +				vmbus_flags,
> +				kick_q);
>  	}
>  
>  	if (ret == 0) {
> diff --git a/drivers/net/hyperv/netvsc_drv.c 
> b/drivers/net/hyperv/netvsc_drv.c
> index a06bd66..80b4b29 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -384,6 +384,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, 
> struct net_device *net)
>  	u32 net_trans_info;
>  	u32 hash;
>  	u32 skb_length = skb->len;
> +	bool kick_q = !skb->xmit_more;
>  
>  
>  	/* We will atmost need two pages to describe the rndis
> @@ -556,7 +557,7 @@ do_send:
>  	packet->page_buf_cnt = init_page_array(rndis_msg, rndis_msg_size,
>  					skb, &packet->page_buf[0]);
>  
> -	ret = netvsc_send(net_device_ctx->device_ctx, packet);
> +	ret = netvsc_send(net_device_ctx->device_ctx, packet, kick_q);
>  
>  drop:
>  	if (ret == 0) {
> diff --git a/drivers/net/hyperv/rndis_filter.c 
> b/drivers/net/hyperv/rndis_filter.c
> index ca81de0..05f3792 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -238,7 +238,7 @@ static int rndis_filter_send_request(struct 
> rndis_device *dev,
>  
>  	packet->send_completion = NULL;
>  
> -	ret = netvsc_send(dev->net_dev->dev, packet);
> +	ret = netvsc_send(dev->net_dev->dev, packet, true);
>  	return ret;
>  }
>  
> -- 
> 1.7.4.1
> 

Re: [PATCH 2/2 net-next] hyperv: Support batched notification

[Olaf Hering]

On Tue, Mar 10, K. Y. Srinivasan wrote:

> Optimize notifying the host by deferring notification until there
> are no more packets to be sent. This will help in batching the requests
> on the host.

The subjects for the network driver say "hyperv:". But all such changes
should get "netvsc:" as prefix to make it clear what each single patch
is all about.

This should be changed for upcoming submissions.

Thanks!

Olaf

RE: [PATCH 2/2 net-next] hyperv: Support batched notification

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3732 bytes --]



> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Tuesday, March 10, 2015 8:34 PM
> To: KY Srinivasan
> Cc: davem@davemloft.net; netdev@vger.kernel.org;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com; KY
> Srinivasan
> Subject: Re: [PATCH 2/2 net-next] hyperv: Support batched notification
> 
> 
> 
> On Wed, Mar 11, 2015 at 2:50 AM, K. Y. Srinivasan <kys@microsoft.com>
> wrote:
> > Optimize notifying the host by deferring notification until there are
> > no more packets to be sent. This will help in batching the requests on
> > the host.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> >  drivers/net/hyperv/hyperv_net.h   |    2 +-
> >  drivers/net/hyperv/netvsc.c       |   14 +++++++++-----
> >  drivers/net/hyperv/netvsc_drv.c   |    3 ++-
> >  drivers/net/hyperv/rndis_filter.c |    2 +-
> >  4 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/hyperv_net.h
> > b/drivers/net/hyperv/hyperv_net.h index 4815843..3fd9896 100644
> > --- a/drivers/net/hyperv/hyperv_net.h
> > +++ b/drivers/net/hyperv/hyperv_net.h
> > @@ -184,7 +184,7 @@ struct rndis_device {  int
> > netvsc_device_add(struct hv_device *device, void *additional_info);
> > int netvsc_device_remove(struct hv_device *device);  int
> > netvsc_send(struct hv_device *device,
> > -		struct hv_netvsc_packet *packet);
> > +		struct hv_netvsc_packet *packet, bool kick_q);
> >  void netvsc_linkstatus_callback(struct hv_device *device_obj,
> >  				struct rndis_message *resp);
> >  int netvsc_recv_callback(struct hv_device *device_obj, diff --git
> > a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index
> > 208eb05..9003b94 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -707,7 +707,7 @@ static u32 netvsc_copy_to_send_buf(struct
> > netvsc_device *net_device,  }
> >
> >  int netvsc_send(struct hv_device *device,
> > -			struct hv_netvsc_packet *packet)
> > +			struct hv_netvsc_packet *packet, bool kick_q)
> >  {
> >  	struct netvsc_device *net_device;
> >  	int ret = 0;
> > @@ -719,6 +719,7 @@ int netvsc_send(struct hv_device *device,
> >  	u32 msg_size = 0;
> >  	struct sk_buff *skb = NULL;
> >  	u16 q_idx = packet->q_idx;
> > +	u32 vmbus_flags =
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED;
> >
> >
> >  	net_device = get_outbound_net_device(device); @@ -768,18
> +769,21 @@
> > int netvsc_send(struct hv_device *device,
> >  		return -ENODEV;
> >
> >  	if (packet->page_buf_cnt) {
> > -		ret = vmbus_sendpacket_pagebuffer(out_channel,
> > +		ret = vmbus_sendpacket_pagebuffer_ctl(out_channel,
> >  						  packet->page_buf,
> >  						  packet->page_buf_cnt,
> >  						  &sendMessage,
> >  						  sizeof(struct
> nvsp_message),
> > -						  req_id);
> > +						  req_id,
> > +						  vmbus_flags,
> > +						  kick_q);
> 
> What if kick_q is false but ret is -EAGAIN here? Looks like in this case host
> won't get notified at all. How about checking whether txq and kicking if it has
> been stopped like what other network driver did?

Good point. I am going to fix this issue in the VMBUS layer. The kick_q argument
is simply a hint to the vmbus level - the lower level can choose not to notify the
host (even if kick_q is true) based on other considerations.

I will resend this series with the logic in the vmbus driver. I will send the patch out
and Greg can decide if the vmbus change should go through Greg's tree or David's tree.

Regards,

K. Y
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 2/2 net-next] hyperv: Support batched notification

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1170 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Wednesday, March 11, 2015 2:24 AM
> To: KY Srinivasan
> Cc: davem@davemloft.net; netdev@vger.kernel.org;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 2/2 net-next] hyperv: Support batched notification
> 
> On Tue, Mar 10, K. Y. Srinivasan wrote:
> 
> > Optimize notifying the host by deferring notification until there are
> > no more packets to be sent. This will help in batching the requests on
> > the host.
> 
> The subjects for the network driver say "hyperv:". But all such changes
> should get "netvsc:" as prefix to make it clear what each single patch is all
> about.
> 
> This should be changed for upcoming submissions.

Olaf,
This is the convention that we have used for patches submitted to David's tree. You will see
net-next in the subject line and you can key off of that.

K. Y
> 
> Thanks!
> 
> Olaf
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/2 net-next] hyperv: Support batched notification

[Olaf Hering]

On Wed, Mar 11, KY Srinivasan wrote:

> This is the convention that we have used for patches submitted to
> David's tree.

I see, it refers to the directory name. In a bunch of backports it gives
the impression it affects the Hyper-V support as whole, while in fact a
given change is just for the network driver. Looks like I have to live
with that.

Olaf