lkml.org 
[lkml]   [2019]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v3 11/12] Drivers: hv: vmbus: Suspend after cleaning up hv_sock and sub channels
Date
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Friday, August 23, 2019 1:17 PM
>
> From: Dexuan Cui Sent: Monday, August 19, 2019 6:52 PM
> >
> > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> > @@ -499,6 +499,8 @@ static void vmbus_add_channel_work(struct
> work_struct *work)
> > return;
> >
> > err_deq_chan:
> > + WARN_ON_ONCE(1);
> > +
>
> Why this change? I was thinking maybe it's a debug statement that got
> left in.

This is indeed a debug statement. :-)
I was not so sure if the error handling is absolutely correct there.
I think I can remove this WARN_ON_ONCE() since almost all of the possible
error paths have a pr_err(). We can make an extra patch to fix any bug in
the existing error handling code. BTW, it should be pretty unlikely to reach
the "err_deq_chan label" in practice.

> > @@ -1021,6 +1044,11 @@ static void vmbus_onoffer_rescind(struct
> > vmbus_channel_message_header *hdr)
> > }
> > mutex_unlock(&vmbus_connection.channel_mutex);
> > }
> > +
> > + /* The "channel" may have been freed. Do not access it any longer. */
> > +
> > + if (clean_up_chan_for_suspend)
> > + check_ready_for_suspend_event();
> > }
>
> Having to add the above lines twice is a bit clumsy, but the problem is
> the overall structure of the vmbus_onoffer_rescind. The early return in
> the case of a rescind_callback function is a bit weird, but I guess it makes
> sense since from what I can tell, only uio and hv_sock have rescind callback
> functions. Some minor restructuring might be warranted, but I don't feel
> strongly about it.

I agree. We should restructure vmbus_onoffer_rescind(), but I think we can
do it in a separate patch. Here in this patch I'm focused on the minimal
required change for hibernation.

> > + /*
> > + * Wait until all the sub-channels and hv_sock channels have been
> > + * cleaned up. Sub-channels should be destroyed upon suspend,
> otherwise
> > + * they would conflict with the new sub-channels that will be created
> > + * in the resume path. hv_sock channels should also be destroyed, but
> > + * a hv_sock channel of an established hv_sock connection can not be
> > + * really destroyed since it may still be referenced by the userspace
> > + * application, so we just force the hv_sock channel to be rescinded
> > + * by vmbus_force_channel_rescinded(), and the userspace application
> > + * will thoroughly destroy the channel after hibernation.
> > + */
> > + if (atomic_read(&vmbus_connection.nr_chan_close_on_suspend) > 0)
>
> At first glance, the above line seemed useless to me. You could just do the
> wait_for_completion() unconditionally. But is the intent to handle the case
> where the VM never had any sub-channels or hv_sock channels, and so
> nr_chan_close_on_suspend never went above 0?

Yes.

> If so, a comment might be helpful.
> > +wait_for_completion(&vmbus_connection.ready_for_suspend_event);

Ok. I'll add a commnt for this in v4.

Thanks,
-- Dexuan

\
 
 \ /
  Last update: 2019-08-31 04:55    [W:0.099 / U:0.748 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site