From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
Abstract vhost acked features saving into
vhost_user_save_acked_features, export it as util function.
Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
---
include/net/vhost-user.h | 2 ++
net/vhost-user.c | 35 +++++++++++++++++++----------------
2 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
index 5bcd8a6..00d4661 100644
--- a/include/net/vhost-user.h
+++ b/include/net/vhost-user.h
@@ -14,5 +14,7 @@
struct vhost_net;
struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
uint64_t vhost_user_get_acked_features(NetClientState *nc);
+void vhost_user_save_acked_features(NetClientState *nc,
+ bool cleanup);
#endif /* VHOST_USER_H */
diff --git a/net/vhost-user.c b/net/vhost-user.c
index b1a0247..c512cc9 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -45,24 +45,31 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
return s->acked_features;
}
-static void vhost_user_stop(int queues, NetClientState *ncs[])
+void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
{
NetVhostUserState *s;
+
+ s = DO_UPCAST(NetVhostUserState, nc, nc);
+ if (s->vhost_net) {
+ uint64_t features = vhost_net_get_acked_features(s->vhost_net);
+ if (features) {
+ s->acked_features = features;
+ }
+
+ if (cleanup) {
+ vhost_net_cleanup(s->vhost_net);
+ }
+ }
+}
+
+static void vhost_user_stop(int queues, NetClientState *ncs[])
+{
int i;
for (i = 0; i < queues; i++) {
assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
- s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
-
- if (s->vhost_net) {
- /* save acked features */
- uint64_t features = vhost_net_get_acked_features(s->vhost_net);
- if (features) {
- s->acked_features = features;
- }
- vhost_net_cleanup(s->vhost_net);
- }
+ vhost_user_save_acked_features(ncs[i], true);
}
}
@@ -251,11 +258,7 @@ static void chr_closed_bh(void *opaque)
s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
for (i = queues -1; i >= 0; i--) {
- s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
-
- if (s->vhost_net) {
- s->acked_features = vhost_net_get_acked_features(s->vhost_net);
- }
+ vhost_user_save_acked_features(ncs[i], false);
}
qmp_set_link(name, false, &err);
--
1.8.3.1
On Sat, Oct 29, 2022 at 01:25:44AM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> Abstract vhost acked features saving into
> vhost_user_save_acked_features, export it as util function.
>
Thanks for the patch!
This commit log makes it sound like it's just a refactoring
while it's actually a behaviour change.
This log needs to include analysis of why is saving only if features != 0
safe.
Could you include that pls?
Thanks!
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
> ---
> include/net/vhost-user.h | 2 ++
> net/vhost-user.c | 35 +++++++++++++++++++----------------
> 2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
> index 5bcd8a6..00d4661 100644
> --- a/include/net/vhost-user.h
> +++ b/include/net/vhost-user.h
> @@ -14,5 +14,7 @@
> struct vhost_net;
> struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
> uint64_t vhost_user_get_acked_features(NetClientState *nc);
> +void vhost_user_save_acked_features(NetClientState *nc,
> + bool cleanup);
>
> #endif /* VHOST_USER_H */
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index b1a0247..c512cc9 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -45,24 +45,31 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
> return s->acked_features;
> }
>
> -static void vhost_user_stop(int queues, NetClientState *ncs[])
> +void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
> {
> NetVhostUserState *s;
> +
> + s = DO_UPCAST(NetVhostUserState, nc, nc);
> + if (s->vhost_net) {
> + uint64_t features = vhost_net_get_acked_features(s->vhost_net);
> + if (features) {
> + s->acked_features = features;
> + }
> +
> + if (cleanup) {
> + vhost_net_cleanup(s->vhost_net);
> + }
> + }
> +}
> +
> +static void vhost_user_stop(int queues, NetClientState *ncs[])
> +{
> int i;
>
> for (i = 0; i < queues; i++) {
> assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
>
> - s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
> -
> - if (s->vhost_net) {
> - /* save acked features */
> - uint64_t features = vhost_net_get_acked_features(s->vhost_net);
> - if (features) {
> - s->acked_features = features;
> - }
> - vhost_net_cleanup(s->vhost_net);
> - }
> + vhost_user_save_acked_features(ncs[i], true);
> }
> }
>
> @@ -251,11 +258,7 @@ static void chr_closed_bh(void *opaque)
> s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>
> for (i = queues -1; i >= 0; i--) {
> - s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
> -
> - if (s->vhost_net) {
> - s->acked_features = vhost_net_get_acked_features(s->vhost_net);
> - }
> + vhost_user_save_acked_features(ncs[i], false);
So this won't do anything if acked features is 0.
When does this have any effect? How about if guest
acked some features, and then reset the device.
Don't we want to reset the features in this case too?
> }
>
> qmp_set_link(name, false, &err);
> --
> 1.8.3.1
在 2022/10/29 16:28, Michael S. Tsirkin 写道:
> On Sat, Oct 29, 2022 at 01:25:44AM +0800, huangy81@chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Abstract vhost acked features saving into
>> vhost_user_save_acked_features, export it as util function.
>>
>
> Thanks for the patch!
>
> This commit log makes it sound like it's just a refactoring
> while it's actually a behaviour change.
> This log needs to include analysis of why is saving only if features != 0
> safe.
>
> Could you include that pls?
>
> Thanks!
>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>> ---
>> include/net/vhost-user.h | 2 ++
>> net/vhost-user.c | 35 +++++++++++++++++++----------------
>> 2 files changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
>> index 5bcd8a6..00d4661 100644
>> --- a/include/net/vhost-user.h
>> +++ b/include/net/vhost-user.h
>> @@ -14,5 +14,7 @@
>> struct vhost_net;
>> struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
>> uint64_t vhost_user_get_acked_features(NetClientState *nc);
>> +void vhost_user_save_acked_features(NetClientState *nc,
>> + bool cleanup);
>>
>> #endif /* VHOST_USER_H */
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index b1a0247..c512cc9 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -45,24 +45,31 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
>> return s->acked_features;
>> }
>>
>> -static void vhost_user_stop(int queues, NetClientState *ncs[])
>> +void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
>> {
>> NetVhostUserState *s;
>> +
>> + s = DO_UPCAST(NetVhostUserState, nc, nc);
>> + if (s->vhost_net) {
>> + uint64_t features = vhost_net_get_acked_features(s->vhost_net);
>> + if (features) {
>> + s->acked_features = features;
>> + }
>> +
>> + if (cleanup) {
>> + vhost_net_cleanup(s->vhost_net);
>> + }
>> + }
>> +}
>> +
>> +static void vhost_user_stop(int queues, NetClientState *ncs[])
>> +{
>> int i;
>>
>> for (i = 0; i < queues; i++) {
>> assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
>>
>> - s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>> -
>> - if (s->vhost_net) {
>> - /* save acked features */
>> - uint64_t features = vhost_net_get_acked_features(s->vhost_net);
>> - if (features) {
>> - s->acked_features = features;
>> - }
>> - vhost_net_cleanup(s->vhost_net);
>> - }
>> + vhost_user_save_acked_features(ncs[i], true);
>> }
>> }
>>
>> @@ -251,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>> s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>>
>> for (i = queues -1; i >= 0; i--) {
>> - s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>> -
>> - if (s->vhost_net) {
>> - s->acked_features = vhost_net_get_acked_features(s->vhost_net);
>> - }
>> + vhost_user_save_acked_features(ncs[i], false);
>
>
> So this won't do anything if acked features is 0.
> When does this have any effect? How about if guest
> acked some features, and then reset the device.
> Don't we want to reset the features in this case too?
>
Sorry about that i just notice that Stefano has replied the question
about "why saving features only if the features aren't 0" 3 weeks ago,
it seems that the answer is not clear.
IMHO, as i metioned in the link:
https://patchew.org/QEMU/20220926063641.25038-1-huangy81@chinatelecom.cn/20220926063641.25038-2-huangy81@chinatelecom.cn/
"Indeed, backing up acked_features in the two functions chr_closed_bh()
vhost_user_stop() are kind of different as above, it also seems a little
weried for me.
IMHO, we can always keep the acked_features in NetVhostUserState the
same as acked_features in vhost_dev no matter what features are, since
this is the role that acked_features in NetVhostUserState plays and we
can just focus on the validation of acked_features in vhost_dev if
something goes wrong"
Maybe we could adopt above strategy and saving acked_features no matter
whether the featuress are 0 or not.
The next version will modify the logic and skip checking features before
saving, meanwhile, i'll post another series for vhost-user-test case to
assert if the acked_features in NetVhostUserState is exactly the same in
vhost slave device, which can check if features is set correctly by
vhost user protocol.
Thanks
Yong
>
>> }
>>
>> qmp_set_link(name, false, &err);
>> --
>> 1.8.3.1
>
--
Best regard
Hyman Huang(黄勇)
在 2022/10/30 13:14, Hyman Huang 写道:
>
>
> 在 2022/10/29 16:28, Michael S. Tsirkin 写道:
>> On Sat, Oct 29, 2022 at 01:25:44AM +0800, huangy81@chinatelecom.cn wrote:
>>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>>
>>> Abstract vhost acked features saving into
>>> vhost_user_save_acked_features, export it as util function.
>>>
>>
>> Thanks for the patch!
>>
>> This commit log makes it sound like it's just a refactoring
>> while it's actually a behaviour change.
>> This log needs to include analysis of why is saving only if features != 0
>> safe.
>>
>> Could you include that pls?
>>
>> Thanks!
>>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>> Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
>>> ---
>>> include/net/vhost-user.h | 2 ++
>>> net/vhost-user.c | 35 +++++++++++++++++++----------------
>>> 2 files changed, 21 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
>>> index 5bcd8a6..00d4661 100644
>>> --- a/include/net/vhost-user.h
>>> +++ b/include/net/vhost-user.h
>>> @@ -14,5 +14,7 @@
>>> struct vhost_net;
>>> struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
>>> uint64_t vhost_user_get_acked_features(NetClientState *nc);
>>> +void vhost_user_save_acked_features(NetClientState *nc,
>>> + bool cleanup);
>>> #endif /* VHOST_USER_H */
>>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>>> index b1a0247..c512cc9 100644
>>> --- a/net/vhost-user.c
>>> +++ b/net/vhost-user.c
>>> @@ -45,24 +45,31 @@ uint64_t
>>> vhost_user_get_acked_features(NetClientState *nc)
>>> return s->acked_features;
>>> }
>>> -static void vhost_user_stop(int queues, NetClientState *ncs[])
>>> +void vhost_user_save_acked_features(NetClientState *nc, bool cleanup)
>>> {
>>> NetVhostUserState *s;
>>> +
>>> + s = DO_UPCAST(NetVhostUserState, nc, nc);
>>> + if (s->vhost_net) {
>>> + uint64_t features = vhost_net_get_acked_features(s->vhost_net);
>>> + if (features) {
>>> + s->acked_features = features;
>>> + }
>>> +
>>> + if (cleanup) {
>>> + vhost_net_cleanup(s->vhost_net);
>>> + }
>>> + }
>>> +}
>>> +
>>> +static void vhost_user_stop(int queues, NetClientState *ncs[])
>>> +{
>>> int i;
>>> for (i = 0; i < queues; i++) {
>>> assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
>>> - s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>>> -
>>> - if (s->vhost_net) {
>>> - /* save acked features */
>>> - uint64_t features =
>>> vhost_net_get_acked_features(s->vhost_net);
>>> - if (features) {
>>> - s->acked_features = features;
>>> - }
>>> - vhost_net_cleanup(s->vhost_net);
>>> - }
>>> + vhost_user_save_acked_features(ncs[i], true);
>>> }
>>> }
>>> @@ -251,11 +258,7 @@ static void chr_closed_bh(void *opaque)
>>> s = DO_UPCAST(NetVhostUserState, nc, ncs[0]);
>>> for (i = queues -1; i >= 0; i--) {
>>> - s = DO_UPCAST(NetVhostUserState, nc, ncs[i]);
>>> -
>>> - if (s->vhost_net) {
>>> - s->acked_features =
>>> vhost_net_get_acked_features(s->vhost_net);
>>> - }
>>> + vhost_user_save_acked_features(ncs[i], false);
>>
>>
>> So this won't do anything if acked features is 0.
>> When does this have any effect? How about if guest
>> acked some features, and then reset the device.
>> Don't we want to reset the features in this case too?
>>
> Sorry about that i just notice that Stefano has replied the question
> about "why saving features only if the features aren't 0" 3 weeks ago,
> it seems that the answer is not clear.
When tring the next version, i seems to find the reason of backing up
acked_features only if the source features aren't 0:
Qemu do not want reset backup negotiated features to 0 and consequently
loss it, let's analyze such process:
1. guest acked virtio-net features
2. Qemu backup it to acked_features in NetVhostUserState
3. vhost slave device unexpected got failed and disconnectted from
master, Qemu update acked_features in chr_closed_bh and free the
vhost_dev, acked_features loss.
4. when vhost slave device show up and Qemu start vhost device again but
failed unexpectedly, vhost_user_stop will called and assign
acked_features in vhost_dev to NetVhostUserState, which are 0, and the
original negotiated features loss.
So i will need to think about if it is reasonable to refactor vhost
acked features saving next version.
Thanks,
Yong
>
> IMHO, as i metioned in the link:
> https://patchew.org/QEMU/20220926063641.25038-1-huangy81@chinatelecom.cn/20220926063641.25038-2-huangy81@chinatelecom.cn/
>
> "Indeed, backing up acked_features in the two functions chr_closed_bh()
> vhost_user_stop() are kind of different as above, it also seems a little
> weried for me.
>
> IMHO, we can always keep the acked_features in NetVhostUserState the
> same as acked_features in vhost_dev no matter what features are, since
> this is the role that acked_features in NetVhostUserState plays and we
> can just focus on the validation of acked_features in vhost_dev if
> something goes wrong"
>
> Maybe we could adopt above strategy and saving acked_features no matter
> whether the featuress are 0 or not.
>
> The next version will modify the logic and skip checking features before
> saving, meanwhile, i'll post another series for vhost-user-test case to
> assert if the acked_features in NetVhostUserState is exactly the same in
> vhost slave device, which can check if features is set correctly by
> vhost user protocol.
>
> Thanks
>
> Yong
>
>>
>>> }
>>> qmp_set_link(name, false, &err);
>>> --
>>> 1.8.3.1
>>
>
--
Best regard
Hyman Huang(黄勇)
© 2016 - 2026 Red Hat, Inc.