hyperv: kvp: wake up the daemon if it's sleeping due to poll()

[decui_microsoft.com]

Summary

Without the patch, there is a race condition: when poll() is invoked(),
if kvp_globals.daemon_busy is false, the daemon won't be timely
woke up, because hv_kvp_send_msg_to_daemon() can't wake up the daemon
in this case.

[delphij]

This changeset looks good to me.

[decui_microsoft.com]

The patch has been in the status of Accepted for 6 days, but I didn't see it appear in the head of the svn repository.

Should I do something extra to make that happen?

This is my first patch for FreeBSD and the process is new to me. :-)

[royger]

AFAICT I think you are missing a:

seldrain(&hv_kvp_selinfo);

In hv_kvp_dev_close before setting kvp_globals.dev_accessed = false.

Also, now that I look at it, the hv_kvp_dev_open function seems to contain a race, you should use atomic_testandset_int in order to test and set kvp_globals.dev_accessed.

[decui_microsoft.com]

In D4258#93144, @royger wrote:

AFAICT I think you are missing a:

seldrain(&hv_kvp_selinfo);

In hv_kvp_dev_close before setting kvp_globals.dev_accessed = false.

Why? At most only 1 daemon is expected to open the dev file /dev/hv_kvp_dev. When the daemon is invoking close() and the execution enters the kernel funntion hv_kvp_dev_close(), we know the daemon is running, so I don't think we need to wake the daemon up?

Also, now that I look at it, the hv_kvp_dev_open function seems to contain a race, you should use atomic_testandset_int in order to test and set kvp_globals.dev_accessed.

Yes, I agree with you. I'll send a separate patch for this.

[decui_microsoft.com]

In D4258#93148, @decui_microsoft.com wrote:

In D4258#93144, @royger wrote:

AFAICT I think you are missing a:

seldrain(&hv_kvp_selinfo);

In hv_kvp_dev_close before setting kvp_globals.dev_accessed = false.

Why? At most only 1 daemon is expected to open the dev file /dev/hv_kvp_dev. When the daemon is invoking close() and the execution enters the kernel funntion hv_kvp_dev_close(), we know the daemon is running, so I don't think we need to wake the daemon up?

Hi Roger, may I have your reply on this?

[royger]

In D4258#93148, @decui_microsoft.com wrote:

In D4258#93144, @royger wrote:

AFAICT I think you are missing a:

seldrain(&hv_kvp_selinfo);

In hv_kvp_dev_close before setting kvp_globals.dev_accessed = false.

Why? At most only 1 daemon is expected to open the dev file /dev/hv_kvp_dev. When the daemon is invoking close() and the execution enters the kernel funntion hv_kvp_dev_close(), we know the daemon is running, so I don't think we need to wake the daemon up?

Sorry for the delay, yesterday was a public holiday here. I agree that you make a special use of this device, so there's no risk in avoiding the seldrain in the close routine (because hv_kvp_selinfo is not going to be freed).

The same applies to hv_kvp_dev_destroy AFAICT, since you kill the thread and don't free the hv_kvp_selinfo struct there's no risk.

Roger.

[decui_microsoft.com]

In D4258#93899, @royger wrote:

In D4258#93148, @decui_microsoft.com wrote:

In D4258#93144, @royger wrote:

AFAICT I think you are missing a:

seldrain(&hv_kvp_selinfo);

In hv_kvp_dev_close before setting kvp_globals.dev_accessed = false.

Why? At most only 1 daemon is expected to open the dev file /dev/hv_kvp_dev. When the daemon is invoking close() and the execution enters the kernel funntion hv_kvp_dev_close(), we know the daemon is running, so I don't think we need to wake the daemon up?

Sorry for the delay, yesterday was a public holiday here. I agree that you make a special use of this device, so there's no risk in avoiding the seldrain in the close routine (because hv_kvp_selinfo is not going to be freed).

The same applies to hv_kvp_dev_destroy AFAICT, since you kill the thread and don't free the hv_kvp_selinfo struct there's no risk.

Roger.

Thanks a lot for the explanation, Roger!

Since we have 2 'accept' s from delphij and you, I suppose the patch will be in head automatically. :-)

BTW, in future we'll carefully clean up the current hv_utils/kvp code , e.g., the race when accessing kvp_globals.dev_accessed.

[royger]

I'm not sure whether Wei will commit this or not. IMHO, we should solve this situation and clarify who is going to commit and maintain HyperV code. Let's see if Wei replies to the "Introducing new FreeBSD developers from Microsoft" and we can clarify the situation and decide how to move forward. If not I will commit it in a couple of days.