From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Andrea Parri <parri.andrea@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
"K . Y . Srinivasan" <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
Sasha Levin <sashal@kernel.org>,
Michael Kelley <mikelley@microsoft.com>,
Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
Date: Tue, 08 Oct 2019 15:00:25 +0200 [thread overview]
Message-ID: <87zhibz91y.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <20191008124052.GA11245@andrea.guest.corp.microsoft.com>
Andrea Parri <parri.andrea@gmail.com> writes:
>> > @@ -244,21 +234,18 @@ int vmbus_connect(void)
>> > * version.
>> > */
>> >
>> > - version = VERSION_CURRENT;
>> > + for (i = 0; ; i++) {
>> > + version = vmbus_versions[i];
>> > + if (version == VERSION_INVAL)
>> > + goto cleanup;
>>
>> If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
>> this code look more natural.
>
> Thank you for pointing this out, Vitaly.
>
> IIUC, you're suggesting that I do:
>
> for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> version = vmbus_versions[i];
>
> ret = vmbus_negotiate_version(msginfo, version);
> if (ret == -ETIMEDOUT)
> goto cleanup;
>
> if (vmbus_connection.conn_state == CONNECTED)
> break;
> }
>
> if (vmbus_connection.conn_state != CONNECTED)
> break;
>
> and that I remove VERSION_INVAL from vmbus_versions, yes?
>
Yes, something like that.
> Looking at the uses of VERSION_INVAL, I find one remaining occurrence
> of this macro in vmbus_bus_resume(), which does:
>
> if (vmbus_proto_version == VERSION_INVAL ||
> vmbus_proto_version == 0) {
> ...
> }
>
> TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
> first time and I'm not sure about how to change such check yet... any
> suggestions?
Hm, I don't think vmbus_proto_version can ever become == VERSION_INVAL
if we rewrite the code the way you suggest, right? So you'll reduce this
to 'if (!vmbus_proto_version)' meaning we haven't negotiated any version
(yet).
>
> Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
> vmbus_connection.conn_state: can such accesses race with a concurrent
> vmbus_connect()?
Let's summon Dexuan who's the author! :-)
>
> Thanks,
> Andrea
>
>
>> >
>> > - do {
>> > ret = vmbus_negotiate_version(msginfo, version);
>> > if (ret == -ETIMEDOUT)
>> > goto cleanup;
>> >
>> > if (vmbus_connection.conn_state == CONNECTED)
>> > break;
>> > -
>> > - version = vmbus_get_next_version(version);
>> > - } while (version != VERSION_INVAL);
>> > -
>> > - if (version == VERSION_INVAL)
>> > - goto cleanup;
>> > + }
>> >
>> > vmbus_proto_version = version;
>> > pr_info("Vmbus version:%d.%d\n",
--
Vitaly
next prev parent reply other threads:[~2019-10-08 13:00 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-07 16:31 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Andrea Parri
2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
2019-10-07 17:14 ` Vitaly Kuznetsov
2019-10-08 12:41 ` Andrea Parri
2019-10-08 12:44 ` Andrea Parri
2019-10-08 13:00 ` Vitaly Kuznetsov [this message]
2019-10-08 22:41 ` Dexuan Cui
2019-10-09 9:54 ` Andrea Parri
2019-10-07 17:25 ` Dexuan Cui
2019-10-08 12:42 ` Andrea Parri
2019-10-07 16:31 ` [PATCH 2/2] Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2 Andrea Parri
2019-10-07 17:41 ` [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Dexuan Cui
2019-10-08 15:08 ` Andrea Parri
2019-10-08 19:47 ` Vitaly Kuznetsov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zhibz91y.fsf@vitty.brq.redhat.com \
--to=vkuznets@redhat.com \
--cc=decui@microsoft.com \
--cc=haiyangz@microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mikelley@microsoft.com \
--cc=parri.andrea@gmail.com \
--cc=sashal@kernel.org \
--cc=sthemmin@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).