Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1366216 > unrolled thread

Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-03-29 14:30 +0200
    Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover Greg KH <gregkh@linuxfoundation.org> - 2016-03-31 23:10 +0200
      RE: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover KY Srinivasan <kys@microsoft.com> - 2016-04-01 23:10 +0200

#1366216 — Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Hyper-V VMs can be replicated to another hosts and there is a feature to
> set different IP for replicas, it is called 'Failover TCP/IP'. When
> such guest starts Hyper-V host sends it KVP_OP_SET_IP_INFO message as soon
> as we finish negotiation procedure. The problem is that it can happen (and
> it actually happens) before userspace daemon connects and we reply with
> HV_E_FAIL to the message. As there are no repetitions we fail to set the
> requested IP.
>
> Solve the issue by postponing our reply to the negotiation message till
> userspace daemon is connected. We can't wait too long as there is a
> host-side timeout (cca. 75 seconds) and if we fail to reply in this time
> frame the whole KVP service will become inactive. The solution is not
> ideal - if it takes userspace daemon more than 60 seconds to connect
> IP Failover will still fail but I don't see a solution with our current
> separation between kernel and userspace parts.
>
> Other two modules (VSS and FCOPY) don't require such delay, leave them
> untouched.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>

An issue was found with this patch: we don't cancel
kvp_host_handshake_work on module unload and if the work is still
pending we end up crashing. As far as I can see this series didn't make
it to char-misc tree so instead of sending a patch to this patch I'll
send v2 for the original issue.

Sorry for the inconvenience.

> ---
>  drivers/hv/hv_kvp.c       |   30 ++++++++++++++++++++++++++++++
>  drivers/hv/hyperv_vmbus.h |    5 +++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index 9b9b370..0d3fcd6 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -78,9 +78,11 @@ static void kvp_send_key(struct work_struct *dummy);
>
>  static void kvp_respond_to_host(struct hv_kvp_msg *msg, int error);
>  static void kvp_timeout_func(struct work_struct *dummy);
> +static void kvp_host_handshake_func(struct work_struct *dummy);
>  static void kvp_register(int);
>
>  static DECLARE_DELAYED_WORK(kvp_timeout_work, kvp_timeout_func);
> +static DECLARE_DELAYED_WORK(kvp_host_handshake_work, kvp_host_handshake_func);
>  static DECLARE_WORK(kvp_sendkey_work, kvp_send_key);
>
>  static const char kvp_devname[] = "vmbus/hv_kvp";
> @@ -130,6 +132,11 @@ static void kvp_timeout_func(struct work_struct *dummy)
>  	hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
>  }
>
> +static void kvp_host_handshake_func(struct work_struct *dummy)
> +{
> +	hv_poll_channel(kvp_transaction.recv_channel, hv_kvp_onchannelcallback);
> +}
> +
>  static int kvp_handle_handshake(struct hv_kvp_msg *msg)
>  {
>  	switch (msg->kvp_hdr.operation) {
> @@ -154,6 +161,12 @@ static int kvp_handle_handshake(struct hv_kvp_msg *msg)
>  	pr_debug("KVP: userspace daemon ver. %d registered\n",
>  		 KVP_OP_REGISTER);
>  	kvp_register(dm_reg_value);
> +
> +	/*
> +	 * If we're still negotiating with the host cancel the timeout
> +	 * work to not poll the channel twice.
> +	 */
> +	cancel_delayed_work_sync(&kvp_host_handshake_work);
>  	hv_poll_channel(kvp_transaction.recv_channel, kvp_poll_wrapper);
>
>  	return 0;
> @@ -594,7 +607,22 @@ void hv_kvp_onchannelcallback(void *context)
>  	struct icmsg_negotiate *negop = NULL;
>  	int util_fw_version;
>  	int kvp_srv_version;
> +	static enum {NEGO_NOT_STARTED,
> +		     NEGO_IN_PROGRESS,
> +		     NEGO_FINISHED} host_negotiatied = NEGO_NOT_STARTED;
>
> +	if (host_negotiatied == NEGO_NOT_STARTED &&
> +	    kvp_transaction.state < HVUTIL_READY) {
> +		/*
> +		 * If userspace daemon is not connected and host is asking
> +		 * us to negotiate we need to delay to not lose messages.
> +		 * This is important for Failover IP setting.
> +		 */
> +		host_negotiatied = NEGO_IN_PROGRESS;
> +		schedule_delayed_work(&kvp_host_handshake_work,
> +				      HV_UTIL_NEGO_TIMEOUT * HZ);
> +		return;
> +	}
>  	if (kvp_transaction.state > HVUTIL_READY)
>  		return;
>
> @@ -672,6 +700,8 @@ void hv_kvp_onchannelcallback(void *context)
>  		vmbus_sendpacket(channel, recv_buffer,
>  				       recvlen, requestid,
>  				       VM_PKT_DATA_INBAND, 0);
> +
> +		host_negotiatied = NEGO_FINISHED;
>  	}
>
>  }
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index a64b176..28e9df9 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -36,6 +36,11 @@
>  #define HV_UTIL_TIMEOUT 30
>
>  /*
> + * Timeout for guest-host handshake for services.
> + */
> +#define HV_UTIL_NEGO_TIMEOUT 60
> +
> +/*
>   * The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent
>   * is set by CPUID(HVCPUID_VERSION_FEATURES).
>   */

-- 
  Vitaly

[toc] | [next] | [standalone]

#1368762

On Tue, Mar 29, 2016 at 02:27:16PM +0200, Vitaly Kuznetsov wrote:
> "K. Y. Srinivasan" <kys@microsoft.com> writes:
> 
> > From: Vitaly Kuznetsov <vkuznets@redhat.com>
> >
> > Hyper-V VMs can be replicated to another hosts and there is a feature to
> > set different IP for replicas, it is called 'Failover TCP/IP'. When
> > such guest starts Hyper-V host sends it KVP_OP_SET_IP_INFO message as soon
> > as we finish negotiation procedure. The problem is that it can happen (and
> > it actually happens) before userspace daemon connects and we reply with
> > HV_E_FAIL to the message. As there are no repetitions we fail to set the
> > requested IP.
> >
> > Solve the issue by postponing our reply to the negotiation message till
> > userspace daemon is connected. We can't wait too long as there is a
> > host-side timeout (cca. 75 seconds) and if we fail to reply in this time
> > frame the whole KVP service will become inactive. The solution is not
> > ideal - if it takes userspace daemon more than 60 seconds to connect
> > IP Failover will still fail but I don't see a solution with our current
> > separation between kernel and userspace parts.
> >
> > Other two modules (VSS and FCOPY) don't require such delay, leave them
> > untouched.
> >
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> 
> An issue was found with this patch: we don't cancel
> kvp_host_handshake_work on module unload and if the work is still
> pending we end up crashing. As far as I can see this series didn't make
> it to char-misc tree so instead of sending a patch to this patch I'll
> send v2 for the original issue.
> 
> Sorry for the inconvenience.

Ok, I'll drop this whole series.  K.Y., can you resend this series after
fixing this up?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1369593

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, March 31, 2016 1:19 PM
> To: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: KY Srinivasan <kys@microsoft.com>; olaf@aepfle.de;
> jasowang@redhat.com; linux-kernel@vger.kernel.org; apw@canonical.com;
> devel@linuxdriverproject.org
> Subject: Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover
> 
> On Tue, Mar 29, 2016 at 02:27:16PM +0200, Vitaly Kuznetsov wrote:
> > "K. Y. Srinivasan" <kys@microsoft.com> writes:
> >
> > > From: Vitaly Kuznetsov <vkuznets@redhat.com>
> > >
> > > Hyper-V VMs can be replicated to another hosts and there is a feature to
> > > set different IP for replicas, it is called 'Failover TCP/IP'. When
> > > such guest starts Hyper-V host sends it KVP_OP_SET_IP_INFO message
> as soon
> > > as we finish negotiation procedure. The problem is that it can happen
> (and
> > > it actually happens) before userspace daemon connects and we reply
> with
> > > HV_E_FAIL to the message. As there are no repetitions we fail to set the
> > > requested IP.
> > >
> > > Solve the issue by postponing our reply to the negotiation message till
> > > userspace daemon is connected. We can't wait too long as there is a
> > > host-side timeout (cca. 75 seconds) and if we fail to reply in this time
> > > frame the whole KVP service will become inactive. The solution is not
> > > ideal - if it takes userspace daemon more than 60 seconds to connect
> > > IP Failover will still fail but I don't see a solution with our current
> > > separation between kernel and userspace parts.
> > >
> > > Other two modules (VSS and FCOPY) don't require such delay, leave
> them
> > > untouched.
> > >
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> >
> > An issue was found with this patch: we don't cancel
> > kvp_host_handshake_work on module unload and if the work is still
> > pending we end up crashing. As far as I can see this series didn't make
> > it to char-misc tree so instead of sending a patch to this patch I'll
> > send v2 for the original issue.
> >
> > Sorry for the inconvenience.
> 
> Ok, I'll drop this whole series.  K.Y., can you resend this series after
> fixing this up?

Will do.

Thanks,

K. Y
> 
> thanks,
> 
> greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web