RE: [PATCH net-next] hyperv: Move state setting for link query

2014-03-05 Thread Haiyang Zhang 


> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Wednesday, March 5, 2014 12:57 AM
> To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
> Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org
> Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
> 
> On 03/05/2014 12:57 AM, Haiyang Zhang wrote:
> >
> >> -Original Message-
> >> From: Jason Wang [mailto:jasow...@redhat.com]
> >> Sent: Monday, March 3, 2014 10:10 PM
> >> To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
> >> Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org;
> >> driverdev- de...@linuxdriverproject.org
> >> Subject: Re: [PATCH net-next] hyperv: Move state setting for link
> >> query
> >>
> >> On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
> >>> It moves the state setting for query into rndis_filter_receive_response().
> >>> All callbacks including query-complete and status-callback are
> >>> synchronized by channel->inbound_lock. This prevents pentential race
> >> between them.
> >>
> >> This still looks racy to me. The problem is workqueue is not
> >> synchronized with those here.
> >>
> >> Consider the following case in netvsc_link_change():
> >>
> >> if (rdev->link_state) {
> >> ... receive interrupt ...
> >> rndis_filter_receice_response() which changes rdev->link_state
> >> ...
> >> netif_carrier_off()
> >> }
> >>
> >> And also it need to schedule a work otherwise the link status is out of 
> >> sync.
> > The rndis_filter_query_device_link_status() makes the query and wait
> > for the complete message, including set state, before returning.
> >
> > The rndis_filter_query_device_link_status() is called from
> > rndis_filter_device_add(), which is called from either netvsc_change_mtu()
> or netvsc_probe().
> >
> > The change_mtu() and netvsc_link_change() are synchronized by
> rtnl_lock().
> > In netvsc_probe(), the status query & complete happens before
> > register_netdev(), and the netvsc_linkstatus_callback() schedules the work
> only after netdevice is registered.
> > So, there are no race in either case.
> >
> > The carrier_on/off work will be scheduled when netvsc_open() is
> > called. Then, the status will be updated based on the latest link_state.
> >
> > Thanks,
> > - Haiyang
> >
> 
> I see. Then if the link status is changing during mtu changing in guest.
> Looks like we may miss a link status updating?

When changing mtu, rndis_filter_device_add() is called, and host will send a 
link
status call back immediately after connect-to-netvsp, no matter the link is 
changing
or not. This will schedule a work to update the status.

Thanks,
- Haiyang

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next] hyperv: Move state setting for link query

2014-03-05 Thread Haiyang Zhang 


 -Original Message-
 From: Jason Wang [mailto:jasow...@redhat.com]
 Sent: Wednesday, March 5, 2014 12:57 AM
 To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
 Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
 de...@linuxdriverproject.org
 Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
 
 On 03/05/2014 12:57 AM, Haiyang Zhang wrote:
 
  -Original Message-
  From: Jason Wang [mailto:jasow...@redhat.com]
  Sent: Monday, March 3, 2014 10:10 PM
  To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
  Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org;
  driverdev- de...@linuxdriverproject.org
  Subject: Re: [PATCH net-next] hyperv: Move state setting for link
  query
 
  On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
  It moves the state setting for query into rndis_filter_receive_response().
  All callbacks including query-complete and status-callback are
  synchronized by channel-inbound_lock. This prevents pentential race
  between them.
 
  This still looks racy to me. The problem is workqueue is not
  synchronized with those here.
 
  Consider the following case in netvsc_link_change():
 
  if (rdev-link_state) {
  ... receive interrupt ...
  rndis_filter_receice_response() which changes rdev-link_state
  ...
  netif_carrier_off()
  }
 
  And also it need to schedule a work otherwise the link status is out of 
  sync.
  The rndis_filter_query_device_link_status() makes the query and wait
  for the complete message, including set state, before returning.
 
  The rndis_filter_query_device_link_status() is called from
  rndis_filter_device_add(), which is called from either netvsc_change_mtu()
 or netvsc_probe().
 
  The change_mtu() and netvsc_link_change() are synchronized by
 rtnl_lock().
  In netvsc_probe(), the status query  complete happens before
  register_netdev(), and the netvsc_linkstatus_callback() schedules the work
 only after netdevice is registered.
  So, there are no race in either case.
 
  The carrier_on/off work will be scheduled when netvsc_open() is
  called. Then, the status will be updated based on the latest link_state.
 
  Thanks,
  - Haiyang
 
 
 I see. Then if the link status is changing during mtu changing in guest.
 Looks like we may miss a link status updating?

When changing mtu, rndis_filter_device_add() is called, and host will send a 
link
status call back immediately after connect-to-netvsp, no matter the link is 
changing
or not. This will schedule a work to update the status.

Thanks,
- Haiyang

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net-next] hyperv: Move state setting for link query

2014-03-04 Thread Jason Wang 
On 03/05/2014 12:57 AM, Haiyang Zhang wrote:
>
>> -Original Message-
>> From: Jason Wang [mailto:jasow...@redhat.com]
>> Sent: Monday, March 3, 2014 10:10 PM
>> To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
>> Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
>> de...@linuxdriverproject.org
>> Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
>>
>> On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
>>> It moves the state setting for query into rndis_filter_receive_response().
>>> All callbacks including query-complete and status-callback are
>>> synchronized by channel->inbound_lock. This prevents pentential race
>> between them.
>>
>> This still looks racy to me. The problem is workqueue is not synchronized 
>> with
>> those here.
>>
>> Consider the following case in netvsc_link_change():
>>
>> if (rdev->link_state) {
>> ... receive interrupt ...
>> rndis_filter_receice_response() which changes rdev->link_state
>> ...
>> netif_carrier_off()
>> }
>>
>> And also it need to schedule a work otherwise the link status is out of sync.
> The rndis_filter_query_device_link_status() makes the query and wait for the
> complete message, including set state, before returning.
>
> The rndis_filter_query_device_link_status() is called from 
> rndis_filter_device_add(),
> which is called from either netvsc_change_mtu() or netvsc_probe().
>
> The change_mtu() and netvsc_link_change() are synchronized by rtnl_lock().
> In netvsc_probe(), the status query & complete happens before 
> register_netdev(), and
> the netvsc_linkstatus_callback() schedules the work only after netdevice is 
> registered.
> So, there are no race in either case.
>
> The carrier_on/off work will be scheduled when netvsc_open() is called. Then,
> the status will be updated based on the latest link_state.
>
> Thanks,
> - Haiyang
>

I see. Then if the link status is changing during mtu changing in guest.
Looks like we may miss a link status updating?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next] hyperv: Move state setting for link query

2014-03-04 Thread Haiyang Zhang 


> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Monday, March 3, 2014 10:10 PM
> To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
> Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org
> Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
> 
> On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
> > It moves the state setting for query into rndis_filter_receive_response().
> > All callbacks including query-complete and status-callback are
> > synchronized by channel->inbound_lock. This prevents pentential race
> between them.
> 
> This still looks racy to me. The problem is workqueue is not synchronized with
> those here.
> 
> Consider the following case in netvsc_link_change():
> 
> if (rdev->link_state) {
> ... receive interrupt ...
> rndis_filter_receice_response() which changes rdev->link_state
> ...
> netif_carrier_off()
> }
> 
> And also it need to schedule a work otherwise the link status is out of sync.

I will update this patch to schedule a work by the end of probe function.

Thanks,
- Haiyang

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next] hyperv: Move state setting for link query

2014-03-04 Thread Haiyang Zhang 


> -Original Message-
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Monday, March 3, 2014 10:10 PM
> To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
> Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
> de...@linuxdriverproject.org
> Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
> 
> On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
> > It moves the state setting for query into rndis_filter_receive_response().
> > All callbacks including query-complete and status-callback are
> > synchronized by channel->inbound_lock. This prevents pentential race
> between them.
> 
> This still looks racy to me. The problem is workqueue is not synchronized with
> those here.
> 
> Consider the following case in netvsc_link_change():
> 
> if (rdev->link_state) {
> ... receive interrupt ...
> rndis_filter_receice_response() which changes rdev->link_state
> ...
> netif_carrier_off()
> }
> 
> And also it need to schedule a work otherwise the link status is out of sync.

The rndis_filter_query_device_link_status() makes the query and wait for the
complete message, including set state, before returning.

The rndis_filter_query_device_link_status() is called from 
rndis_filter_device_add(),
which is called from either netvsc_change_mtu() or netvsc_probe().

The change_mtu() and netvsc_link_change() are synchronized by rtnl_lock().
In netvsc_probe(), the status query & complete happens before 
register_netdev(), and
the netvsc_linkstatus_callback() schedules the work only after netdevice is 
registered.
So, there are no race in either case.

The carrier_on/off work will be scheduled when netvsc_open() is called. Then,
the status will be updated based on the latest link_state.

Thanks,
- Haiyang

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next] hyperv: Move state setting for link query

2014-03-04 Thread Haiyang Zhang 


 -Original Message-
 From: Jason Wang [mailto:jasow...@redhat.com]
 Sent: Monday, March 3, 2014 10:10 PM
 To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
 Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
 de...@linuxdriverproject.org
 Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
 
 On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
  It moves the state setting for query into rndis_filter_receive_response().
  All callbacks including query-complete and status-callback are
  synchronized by channel-inbound_lock. This prevents pentential race
 between them.
 
 This still looks racy to me. The problem is workqueue is not synchronized with
 those here.
 
 Consider the following case in netvsc_link_change():
 
 if (rdev-link_state) {
 ... receive interrupt ...
 rndis_filter_receice_response() which changes rdev-link_state
 ...
 netif_carrier_off()
 }
 
 And also it need to schedule a work otherwise the link status is out of sync.

The rndis_filter_query_device_link_status() makes the query and wait for the
complete message, including set state, before returning.

The rndis_filter_query_device_link_status() is called from 
rndis_filter_device_add(),
which is called from either netvsc_change_mtu() or netvsc_probe().

The change_mtu() and netvsc_link_change() are synchronized by rtnl_lock().
In netvsc_probe(), the status query  complete happens before 
register_netdev(), and
the netvsc_linkstatus_callback() schedules the work only after netdevice is 
registered.
So, there are no race in either case.

The carrier_on/off work will be scheduled when netvsc_open() is called. Then,
the status will be updated based on the latest link_state.

Thanks,
- Haiyang

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH net-next] hyperv: Move state setting for link query

2014-03-04 Thread Haiyang Zhang 


 -Original Message-
 From: Jason Wang [mailto:jasow...@redhat.com]
 Sent: Monday, March 3, 2014 10:10 PM
 To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
 Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
 de...@linuxdriverproject.org
 Subject: Re: [PATCH net-next] hyperv: Move state setting for link query
 
 On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
  It moves the state setting for query into rndis_filter_receive_response().
  All callbacks including query-complete and status-callback are
  synchronized by channel-inbound_lock. This prevents pentential race
 between them.
 
 This still looks racy to me. The problem is workqueue is not synchronized with
 those here.
 
 Consider the following case in netvsc_link_change():
 
 if (rdev-link_state) {
 ... receive interrupt ...
 rndis_filter_receice_response() which changes rdev-link_state
 ...
 netif_carrier_off()
 }
 
 And also it need to schedule a work otherwise the link status is out of sync.

I will update this patch to schedule a work by the end of probe function.

Thanks,
- Haiyang

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net-next] hyperv: Move state setting for link query

2014-03-04 Thread Jason Wang 
On 03/05/2014 12:57 AM, Haiyang Zhang wrote:

 -Original Message-
 From: Jason Wang [mailto:jasow...@redhat.com]
 Sent: Monday, March 3, 2014 10:10 PM
 To: Haiyang Zhang; da...@davemloft.net; net...@vger.kernel.org
 Cc: KY Srinivasan; o...@aepfle.de; linux-kernel@vger.kernel.org; driverdev-
 de...@linuxdriverproject.org
 Subject: Re: [PATCH net-next] hyperv: Move state setting for link query

 On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
 It moves the state setting for query into rndis_filter_receive_response().
 All callbacks including query-complete and status-callback are
 synchronized by channel-inbound_lock. This prevents pentential race
 between them.

 This still looks racy to me. The problem is workqueue is not synchronized 
 with
 those here.

 Consider the following case in netvsc_link_change():

 if (rdev-link_state) {
 ... receive interrupt ...
 rndis_filter_receice_response() which changes rdev-link_state
 ...
 netif_carrier_off()
 }

 And also it need to schedule a work otherwise the link status is out of sync.
 The rndis_filter_query_device_link_status() makes the query and wait for the
 complete message, including set state, before returning.

 The rndis_filter_query_device_link_status() is called from 
 rndis_filter_device_add(),
 which is called from either netvsc_change_mtu() or netvsc_probe().

 The change_mtu() and netvsc_link_change() are synchronized by rtnl_lock().
 In netvsc_probe(), the status query  complete happens before 
 register_netdev(), and
 the netvsc_linkstatus_callback() schedules the work only after netdevice is 
 registered.
 So, there are no race in either case.

 The carrier_on/off work will be scheduled when netvsc_open() is called. Then,
 the status will be updated based on the latest link_state.

 Thanks,
 - Haiyang


I see. Then if the link status is changing during mtu changing in guest.
Looks like we may miss a link status updating?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net-next] hyperv: Move state setting for link query

2014-03-03 Thread Jason Wang 
On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
> It moves the state setting for query into rndis_filter_receive_response().
> All callbacks including query-complete and status-callback are synchronized
> by channel->inbound_lock. This prevents pentential race between them.

This still looks racy to me. The problem is workqueue is not
synchronized with those here.

Consider the following case in netvsc_link_change():

if (rdev->link_state) {
... receive interrupt ...
rndis_filter_receice_response() which changes rdev->link_state
...
netif_carrier_off()
}

And also it need to schedule a work otherwise the link status is out of
sync.
> Signed-off-by: Haiyang Zhang 
> ---
>  drivers/net/hyperv/rndis_filter.c |   21 -
>  1 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/hyperv/rndis_filter.c 
> b/drivers/net/hyperv/rndis_filter.c
> index f0cc8ef..6a9f602 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -240,6 +240,22 @@ static int rndis_filter_send_request(struct rndis_device 
> *dev,
>   return ret;
>  }
>  
> +static void rndis_set_link_state(struct rndis_device *rdev,
> +  struct rndis_request *request)
> +{
> + u32 link_status;
> + struct rndis_query_complete *query_complete;
> +
> + query_complete = >response_msg.msg.query_complete;
> +
> + if (query_complete->status == RNDIS_STATUS_SUCCESS &&
> + query_complete->info_buflen == sizeof(u32)) {
> + memcpy(_status, (void *)((unsigned long)query_complete +
> +query_complete->info_buf_offset), sizeof(u32));
> + rdev->link_state = link_status != 0;
> + }
> +}
> +
>  static void rndis_filter_receive_response(struct rndis_device *dev,
>  struct rndis_message *resp)
>  {
> @@ -269,6 +285,10 @@ static void rndis_filter_receive_response(struct 
> rndis_device *dev,
>   sizeof(struct rndis_message) + RNDIS_EXT_LEN) {
>   memcpy(>response_msg, resp,
>  resp->msg_len);
> + if (request->request_msg.ndis_msg_type ==
> + RNDIS_MSG_QUERY && request->request_msg.msg.
> + query_req.oid == RNDIS_OID_GEN_MEDIA_CONNECT_STATUS)
> + rndis_set_link_state(dev, request);
>   } else {
>   netdev_err(ndev,
>   "rndis response buffer overflow "
> @@ -617,7 +637,6 @@ static int rndis_filter_query_device_link_status(struct 
> rndis_device *dev)
>   ret = rndis_filter_query_device(dev,
> RNDIS_OID_GEN_MEDIA_CONNECT_STATUS,
> _status, );
> - dev->link_state = (link_status != 0) ? true : false;
>  
>   return ret;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH net-next] hyperv: Move state setting for link query

2014-03-03 Thread Jason Wang 
On 03/04/2014 07:54 AM, Haiyang Zhang wrote:
 It moves the state setting for query into rndis_filter_receive_response().
 All callbacks including query-complete and status-callback are synchronized
 by channel-inbound_lock. This prevents pentential race between them.

This still looks racy to me. The problem is workqueue is not
synchronized with those here.

Consider the following case in netvsc_link_change():

if (rdev-link_state) {
... receive interrupt ...
rndis_filter_receice_response() which changes rdev-link_state
...
netif_carrier_off()
}

And also it need to schedule a work otherwise the link status is out of
sync.
 Signed-off-by: Haiyang Zhang haiya...@microsoft.com
 ---
  drivers/net/hyperv/rndis_filter.c |   21 -
  1 files changed, 20 insertions(+), 1 deletions(-)

 diff --git a/drivers/net/hyperv/rndis_filter.c 
 b/drivers/net/hyperv/rndis_filter.c
 index f0cc8ef..6a9f602 100644
 --- a/drivers/net/hyperv/rndis_filter.c
 +++ b/drivers/net/hyperv/rndis_filter.c
 @@ -240,6 +240,22 @@ static int rndis_filter_send_request(struct rndis_device 
 *dev,
   return ret;
  }
  
 +static void rndis_set_link_state(struct rndis_device *rdev,
 +  struct rndis_request *request)
 +{
 + u32 link_status;
 + struct rndis_query_complete *query_complete;
 +
 + query_complete = request-response_msg.msg.query_complete;
 +
 + if (query_complete-status == RNDIS_STATUS_SUCCESS 
 + query_complete-info_buflen == sizeof(u32)) {
 + memcpy(link_status, (void *)((unsigned long)query_complete +
 +query_complete-info_buf_offset), sizeof(u32));
 + rdev-link_state = link_status != 0;
 + }
 +}
 +
  static void rndis_filter_receive_response(struct rndis_device *dev,
  struct rndis_message *resp)
  {
 @@ -269,6 +285,10 @@ static void rndis_filter_receive_response(struct 
 rndis_device *dev,
   sizeof(struct rndis_message) + RNDIS_EXT_LEN) {
   memcpy(request-response_msg, resp,
  resp-msg_len);
 + if (request-request_msg.ndis_msg_type ==
 + RNDIS_MSG_QUERY  request-request_msg.msg.
 + query_req.oid == RNDIS_OID_GEN_MEDIA_CONNECT_STATUS)
 + rndis_set_link_state(dev, request);
   } else {
   netdev_err(ndev,
   rndis response buffer overflow 
 @@ -617,7 +637,6 @@ static int rndis_filter_query_device_link_status(struct 
 rndis_device *dev)
   ret = rndis_filter_query_device(dev,
 RNDIS_OID_GEN_MEDIA_CONNECT_STATUS,
 link_status, size);
 - dev-link_state = (link_status != 0) ? true : false;
  
   return ret;
  }

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/