[PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Laurent Vivier]

In vhost_user_receive() if vhost_net_notify_migration_done() reports
an error we display on the console:

  Vhost user backend fails to broadcast fake RARP

This message can be useful if there is a problem to execute
VHOST_USER_SEND_RARP but it is useless if the backend doesn't
support VHOST_USER_PROTOCOL_F_RARP.

Don't report the error if vhost_net_notify_migration_done()
returns -ENOTSUP (from vhost_user_migration_done())

Update vhost_net-stub.c to return -ENOTSUP too.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/net/vhost_net-stub.c | 2 +-
 net/vhost-user.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
index 72df6d757e4d..875cd6c2b9c8 100644
--- a/hw/net/vhost_net-stub.c
+++ b/hw/net/vhost_net-stub.c
@@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
 
 int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
 {
-    return -1;
+    return -ENOTSUP;
 }
 
 VHostNetState *get_vhost_net(NetClientState *nc)
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 12555518e838..636fff8a84a2 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -146,7 +146,7 @@ static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t *buf,
 
         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
 
-        if ((r != 0) && (display_rarp_failure)) {
+        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
             fprintf(stderr,
                     "Vhost user backend fails to broadcast fake RARP\n");
             fflush(stderr);
-- 
2.47.1

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Stefano Garzarella]

On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
>In vhost_user_receive() if vhost_net_notify_migration_done() reports
>an error we display on the console:
>
>  Vhost user backend fails to broadcast fake RARP
>
>This message can be useful if there is a problem to execute
>VHOST_USER_SEND_RARP but it is useless if the backend doesn't
>support VHOST_USER_PROTOCOL_F_RARP.
>
>Don't report the error if vhost_net_notify_migration_done()
>returns -ENOTSUP (from vhost_user_migration_done())
>
>Update vhost_net-stub.c to return -ENOTSUP too.
>
>Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>---
> hw/net/vhost_net-stub.c | 2 +-
> net/vhost-user.c        | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
>index 72df6d757e4d..875cd6c2b9c8 100644
>--- a/hw/net/vhost_net-stub.c
>+++ b/hw/net/vhost_net-stub.c
>@@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
>
> int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
> {
>-    return -1;
>+    return -ENOTSUP;
> }
>
> VHostNetState *get_vhost_net(NetClientState *nc)
>diff --git a/net/vhost-user.c b/net/vhost-user.c
>index 12555518e838..636fff8a84a2 100644
>--- a/net/vhost-user.c
>+++ b/net/vhost-user.c
>@@ -146,7 +146,7 @@ static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t *buf,
>
>         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
>
>-        if ((r != 0) && (display_rarp_failure)) {
>+        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
>             fprintf(stderr,
>                     "Vhost user backend fails to broadcast fake RARP\n");
>             fflush(stderr);
>-- 
>2.47.1
>

IIUC the message was there since the introduction about 10 years ago
from commit 3e866365e1 ("vhost user: add rarp sending after live
migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
and F_GUEST_ANNOUNCE are not negotiated.

That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
but my understanding is that the message was to notify that the
migration was finished (reading that commit).

If neither feature is supported, could this be a problem for the user
and that's why we were printing the message?

Thanks,
Stefano

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Michael S. Tsirkin]

On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
> On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
> > In vhost_user_receive() if vhost_net_notify_migration_done() reports
> > an error we display on the console:
> > 
> >  Vhost user backend fails to broadcast fake RARP
> > 
> > This message can be useful if there is a problem to execute
> > VHOST_USER_SEND_RARP but it is useless if the backend doesn't
> > support VHOST_USER_PROTOCOL_F_RARP.
> > 
> > Don't report the error if vhost_net_notify_migration_done()
> > returns -ENOTSUP (from vhost_user_migration_done())
> > 
> > Update vhost_net-stub.c to return -ENOTSUP too.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> > hw/net/vhost_net-stub.c | 2 +-
> > net/vhost-user.c        | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
> > index 72df6d757e4d..875cd6c2b9c8 100644
> > --- a/hw/net/vhost_net-stub.c
> > +++ b/hw/net/vhost_net-stub.c
> > @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
> > 
> > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
> > {
> > -    return -1;
> > +    return -ENOTSUP;
> > }
> > 
> > VHostNetState *get_vhost_net(NetClientState *nc)
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 12555518e838..636fff8a84a2 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -146,7 +146,7 @@ static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t *buf,
> > 
> >         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
> > 
> > -        if ((r != 0) && (display_rarp_failure)) {
> > +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
> >             fprintf(stderr,
> >                     "Vhost user backend fails to broadcast fake RARP\n");
> >             fflush(stderr);
> > -- 
> > 2.47.1
> > 
> 
> IIUC the message was there since the introduction about 10 years ago
> from commit 3e866365e1 ("vhost user: add rarp sending after live
> migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
> and F_GUEST_ANNOUNCE are not negotiated.
> 
> That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,

rarp is to have destination host broadcast a message with VM address
to update the network. Guest announce is when it will instead
ask the guest to do this.


> but my understanding is that the message was to notify that the
> migration was finished (reading that commit).
> 
> If neither feature is supported, could this be a problem for the user
> and that's why we were printing the message?
> 
> Thanks,
> Stefano

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Stefano Garzarella]

On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
>On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
>> On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
>> > In vhost_user_receive() if vhost_net_notify_migration_done() reports
>> > an error we display on the console:
>> >
>> >  Vhost user backend fails to broadcast fake RARP
>> >
>> > This message can be useful if there is a problem to execute
>> > VHOST_USER_SEND_RARP but it is useless if the backend doesn't
>> > support VHOST_USER_PROTOCOL_F_RARP.
>> >
>> > Don't report the error if vhost_net_notify_migration_done()
>> > returns -ENOTSUP (from vhost_user_migration_done())
>> >
>> > Update vhost_net-stub.c to return -ENOTSUP too.
>> >
>> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> > ---
>> > hw/net/vhost_net-stub.c | 2 +-
>> > net/vhost-user.c        | 2 +-
>> > 2 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
>> > index 72df6d757e4d..875cd6c2b9c8 100644
>> > --- a/hw/net/vhost_net-stub.c
>> > +++ b/hw/net/vhost_net-stub.c
>> > @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
>> >
>> > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
>> > {
>> > -    return -1;
>> > +    return -ENOTSUP;
>> > }
>> >
>> > VHostNetState *get_vhost_net(NetClientState *nc)
>> > diff --git a/net/vhost-user.c b/net/vhost-user.c
>> > index 12555518e838..636fff8a84a2 100644
>> > --- a/net/vhost-user.c
>> > +++ b/net/vhost-user.c
>> > @@ -146,7 +146,7 @@ static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t *buf,
>> >
>> >         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
>> >
>> > -        if ((r != 0) && (display_rarp_failure)) {
>> > +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
>> >             fprintf(stderr,
>> >                     "Vhost user backend fails to broadcast fake RARP\n");
>> >             fflush(stderr);
>> > --
>> > 2.47.1
>> >
>>
>> IIUC the message was there since the introduction about 10 years ago
>> from commit 3e866365e1 ("vhost user: add rarp sending after live
>> migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
>> and F_GUEST_ANNOUNCE are not negotiated.
>>
>> That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
>
>rarp is to have destination host broadcast a message with VM address
>to update the network. Guest announce is when it will instead
>ask the guest to do this.

Okay, thanks for explaining to me.
So if both features are not negotiated, no one is going to broadcast
the message, right?

Could that be a valid reason to print an error message in QEMU?

To me it might be reasonable because the user might experience some
network problems, but I'm not a network guy :-)

Thanks,
Stefano

>
>
>> but my understanding is that the message was to notify that the
>> migration was finished (reading that commit).
>>
>> If neither feature is supported, could this be a problem for the user
>> and that's why we were printing the message?
>>
>> Thanks,
>> Stefano
>

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Michael S. Tsirkin]

On Wed, Jan 22, 2025 at 05:20:06PM +0100, Stefano Garzarella wrote:
> On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
> > > On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
> > > > In vhost_user_receive() if vhost_net_notify_migration_done() reports
> > > > an error we display on the console:
> > > >
> > > >  Vhost user backend fails to broadcast fake RARP
> > > >
> > > > This message can be useful if there is a problem to execute
> > > > VHOST_USER_SEND_RARP but it is useless if the backend doesn't
> > > > support VHOST_USER_PROTOCOL_F_RARP.
> > > >
> > > > Don't report the error if vhost_net_notify_migration_done()
> > > > returns -ENOTSUP (from vhost_user_migration_done())
> > > >
> > > > Update vhost_net-stub.c to return -ENOTSUP too.
> > > >
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > ---
> > > > hw/net/vhost_net-stub.c | 2 +-
> > > > net/vhost-user.c        | 2 +-
> > > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
> > > > index 72df6d757e4d..875cd6c2b9c8 100644
> > > > --- a/hw/net/vhost_net-stub.c
> > > > +++ b/hw/net/vhost_net-stub.c
> > > > @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, bool mask)
> > > >
> > > > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
> > > > {
> > > > -    return -1;
> > > > +    return -ENOTSUP;
> > > > }
> > > >
> > > > VHostNetState *get_vhost_net(NetClientState *nc)
> > > > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > > > index 12555518e838..636fff8a84a2 100644
> > > > --- a/net/vhost-user.c
> > > > +++ b/net/vhost-user.c
> > > > @@ -146,7 +146,7 @@ static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t *buf,
> > > >
> > > >         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
> > > >
> > > > -        if ((r != 0) && (display_rarp_failure)) {
> > > > +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
> > > >             fprintf(stderr,
> > > >                     "Vhost user backend fails to broadcast fake RARP\n");
> > > >             fflush(stderr);
> > > > --
> > > > 2.47.1
> > > >
> > > 
> > > IIUC the message was there since the introduction about 10 years ago
> > > from commit 3e866365e1 ("vhost user: add rarp sending after live
> > > migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
> > > and F_GUEST_ANNOUNCE are not negotiated.
> > > 
> > > That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
> > 
> > rarp is to have destination host broadcast a message with VM address
> > to update the network. Guest announce is when it will instead
> > ask the guest to do this.
> 
> Okay, thanks for explaining to me.
> So if both features are not negotiated, no one is going to broadcast
> the message, right?
> 
> Could that be a valid reason to print an error message in QEMU?
> 
> To me it might be reasonable because the user might experience some
> network problems, but I'm not a network guy :-)
> 
> Thanks,
> Stefano

reasonable, yes.

> > 
> > 
> > > but my understanding is that the message was to notify that the
> > > migration was finished (reading that commit).
> > > 
> > > If neither feature is supported, could this be a problem for the user
> > > and that's why we were printing the message?
> > > 
> > > Thanks,
> > > Stefano
> > 

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Laurent Vivier]

On 22/01/2025 17:20, Stefano Garzarella wrote:
> On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
>>> On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
>>> > In vhost_user_receive() if vhost_net_notify_migration_done() reports
>>> > an error we display on the console:
>>> >
>>> >  Vhost user backend fails to broadcast fake RARP
>>> >
>>> > This message can be useful if there is a problem to execute
>>> > VHOST_USER_SEND_RARP but it is useless if the backend doesn't
>>> > support VHOST_USER_PROTOCOL_F_RARP.
>>> >
>>> > Don't report the error if vhost_net_notify_migration_done()
>>> > returns -ENOTSUP (from vhost_user_migration_done())
>>> >
>>> > Update vhost_net-stub.c to return -ENOTSUP too.
>>> >
>>> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> > ---
>>> > hw/net/vhost_net-stub.c | 2 +-
>>> > net/vhost-user.c        | 2 +-
>>> > 2 files changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
>>> > index 72df6d757e4d..875cd6c2b9c8 100644
>>> > --- a/hw/net/vhost_net-stub.c
>>> > +++ b/hw/net/vhost_net-stub.c
>>> > @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState *net, VirtIODevice *dev, 
>>> bool mask)
>>> >
>>> > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
>>> > {
>>> > -    return -1;
>>> > +    return -ENOTSUP;
>>> > }
>>> >
>>> > VHostNetState *get_vhost_net(NetClientState *nc)
>>> > diff --git a/net/vhost-user.c b/net/vhost-user.c
>>> > index 12555518e838..636fff8a84a2 100644
>>> > --- a/net/vhost-user.c
>>> > +++ b/net/vhost-user.c
>>> > @@ -146,7 +146,7 @@ static ssize_t vhost_user_receive(NetClientState *nc, const 
>>> uint8_t *buf,
>>> >
>>> >         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
>>> >
>>> > -        if ((r != 0) && (display_rarp_failure)) {
>>> > +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
>>> >             fprintf(stderr,
>>> >                     "Vhost user backend fails to broadcast fake RARP\n");
>>> >             fflush(stderr);
>>> > --
>>> > 2.47.1
>>> >
>>>
>>> IIUC the message was there since the introduction about 10 years ago
>>> from commit 3e866365e1 ("vhost user: add rarp sending after live
>>> migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
>>> and F_GUEST_ANNOUNCE are not negotiated.
>>>
>>> That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
>>
>> rarp is to have destination host broadcast a message with VM address
>> to update the network. Guest announce is when it will instead
>> ask the guest to do this.
> 
> Okay, thanks for explaining to me.
> So if both features are not negotiated, no one is going to broadcast
> the message, right?
> 
> Could that be a valid reason to print an error message in QEMU?
> 
> To me it might be reasonable because the user might experience some
> network problems, but I'm not a network guy :-)

I'm working on adding vhost-user to passt[1], and in this case we don't need to broadcast 
any message.

So I don't implement VHOST_USER_SEND_RARP and I don't want the error message to spoil my 
console.

-ENOTSUP is an error message for developer not for user.

Thanks,
Laurent
[1] https://passt.top/passt/

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Stefano Garzarella]

On Wed, Jan 22, 2025 at 05:41:15PM +0100, Laurent Vivier wrote:
>On 22/01/2025 17:20, Stefano Garzarella wrote:
>>On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
>>>On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
>>>>On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
>>>>> In vhost_user_receive() if vhost_net_notify_migration_done() reports
>>>>> an error we display on the console:
>>>>>
>>>>>  Vhost user backend fails to broadcast fake RARP
>>>>>
>>>>> This message can be useful if there is a problem to execute
>>>>> VHOST_USER_SEND_RARP but it is useless if the backend doesn't
>>>>> support VHOST_USER_PROTOCOL_F_RARP.
>>>>>
>>>>> Don't report the error if vhost_net_notify_migration_done()
>>>>> returns -ENOTSUP (from vhost_user_migration_done())
>>>>>
>>>>> Update vhost_net-stub.c to return -ENOTSUP too.
>>>>>
>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>> ---
>>>>> hw/net/vhost_net-stub.c | 2 +-
>>>>> net/vhost-user.c        | 2 +-
>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
>>>>> index 72df6d757e4d..875cd6c2b9c8 100644
>>>>> --- a/hw/net/vhost_net-stub.c
>>>>> +++ b/hw/net/vhost_net-stub.c
>>>>> @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState 
>>>>*net, VirtIODevice *dev, bool mask)
>>>>>
>>>>> int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
>>>>> {
>>>>> -    return -1;
>>>>> +    return -ENOTSUP;
>>>>> }
>>>>>
>>>>> VHostNetState *get_vhost_net(NetClientState *nc)
>>>>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>>>>> index 12555518e838..636fff8a84a2 100644
>>>>> --- a/net/vhost-user.c
>>>>> +++ b/net/vhost-user.c
>>>>> @@ -146,7 +146,7 @@ static ssize_t 
>>>>vhost_user_receive(NetClientState *nc, const uint8_t *buf,
>>>>>
>>>>>         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
>>>>>
>>>>> -        if ((r != 0) && (display_rarp_failure)) {
>>>>> +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
>>>>>             fprintf(stderr,
>>>>>                     "Vhost user backend fails to broadcast fake RARP\n");
>>>>>             fflush(stderr);
>>>>> --
>>>>> 2.47.1
>>>>>
>>>>
>>>>IIUC the message was there since the introduction about 10 years ago
>>>>from commit 3e866365e1 ("vhost user: add rarp sending after live
>>>>migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
>>>>and F_GUEST_ANNOUNCE are not negotiated.
>>>>
>>>>That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
>>>
>>>rarp is to have destination host broadcast a message with VM address
>>>to update the network. Guest announce is when it will instead
>>>ask the guest to do this.
>>
>>Okay, thanks for explaining to me.
>>So if both features are not negotiated, no one is going to broadcast
>>the message, right?
>>
>>Could that be a valid reason to print an error message in QEMU?
>>
>>To me it might be reasonable because the user might experience some
>>network problems, but I'm not a network guy :-)
>
>I'm working on adding vhost-user to passt[1], and in this case we 
>don't need to broadcast any message.

Okay, so please can you add that to the commit description and also
explaining why you don't need that?

>
>So I don't implement VHOST_USER_SEND_RARP and I don't want the error 
>message to spoil my console.

Fair enough, but at that point, if it's valid to have both feature not
negotiated, IMHO is better to return 0 in vhost_user_migration_done().
Maybe adding also a comment to explain that in your scenario you don't
need to do nothing (like if guest supports GUEST_ANNOUNCE).

>
>-ENOTSUP is an error message for developer not for user.

I was referring to the "Vhost user backend fails to broadcast fake RARP"
error message we are skipping here.

Thanks,
Stefano

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Laurent Vivier]

On 22/01/2025 17:51, Stefano Garzarella wrote:
> On Wed, Jan 22, 2025 at 05:41:15PM +0100, Laurent Vivier wrote:
>> On 22/01/2025 17:20, Stefano Garzarella wrote:
>>> On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
>>>> On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
>>>>> On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
>>>>>> In vhost_user_receive() if vhost_net_notify_migration_done() reports
>>>>>> an error we display on the console:
>>>>>>
>>>>>>   Vhost user backend fails to broadcast fake RARP
>>>>>>
>>>>>> This message can be useful if there is a problem to execute
>>>>>> VHOST_USER_SEND_RARP but it is useless if the backend doesn't
>>>>>> support VHOST_USER_PROTOCOL_F_RARP.
>>>>>>
>>>>>> Don't report the error if vhost_net_notify_migration_done()
>>>>>> returns -ENOTSUP (from vhost_user_migration_done())
>>>>>>
>>>>>> Update vhost_net-stub.c to return -ENOTSUP too.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>> ---
>>>>>> hw/net/vhost_net-stub.c | 2 +-
>>>>>> net/vhost-user.c        | 2 +-
>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
>>>>>> index 72df6d757e4d..875cd6c2b9c8 100644
>>>>>> --- a/hw/net/vhost_net-stub.c
>>>>>> +++ b/hw/net/vhost_net-stub.c
>>>>>> @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState 
>>>>> *net, VirtIODevice *dev, bool mask)
>>>>>>
>>>>>> int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
>>>>>> {
>>>>>> -    return -1;
>>>>>> +    return -ENOTSUP;
>>>>>> }
>>>>>>
>>>>>> VHostNetState *get_vhost_net(NetClientState *nc)
>>>>>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>>>>>> index 12555518e838..636fff8a84a2 100644
>>>>>> --- a/net/vhost-user.c
>>>>>> +++ b/net/vhost-user.c
>>>>>> @@ -146,7 +146,7 @@ static ssize_t 
>>>>> vhost_user_receive(NetClientState *nc, const uint8_t *buf,
>>>>>>
>>>>>>          r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
>>>>>>
>>>>>> -        if ((r != 0) && (display_rarp_failure)) {
>>>>>> +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
>>>>>>              fprintf(stderr,
>>>>>>                      "Vhost user backend fails to broadcast fake RARP\n");
>>>>>>              fflush(stderr);
>>>>>> -- 
>>>>>> 2.47.1
>>>>>>
>>>>>
>>>>> IIUC the message was there since the introduction about 10 years ago
>>>>> from commit 3e866365e1 ("vhost user: add rarp sending after live
>>>>> migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
>>>>> and F_GUEST_ANNOUNCE are not negotiated.
>>>>>
>>>>> That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
>>>>
>>>> rarp is to have destination host broadcast a message with VM address
>>>> to update the network. Guest announce is when it will instead
>>>> ask the guest to do this.
>>>
>>> Okay, thanks for explaining to me.
>>> So if both features are not negotiated, no one is going to broadcast
>>> the message, right?
>>>
>>> Could that be a valid reason to print an error message in QEMU?
>>>
>>> To me it might be reasonable because the user might experience some
>>> network problems, but I'm not a network guy :-)
>>
>> I'm working on adding vhost-user to passt[1], and in this case we don't need to 
>> broadcast any message.
> 
> Okay, so please can you add that to the commit description and also
> explaining why you don't need that?
> 
>>
>> So I don't implement VHOST_USER_SEND_RARP and I don't want the error message to spoil my 
>> console.
> 
> Fair enough, but at that point, if it's valid to have both feature not
> negotiated, IMHO is better to return 0 in vhost_user_migration_done().
> Maybe adding also a comment to explain that in your scenario you don't
> need to do nothing (like if guest supports GUEST_ANNOUNCE).

I agree.

> 
>>
>> -ENOTSUP is an error message for developer not for user.
> 
> I was referring to the "Vhost user backend fails to broadcast fake RARP"
> error message we are skipping here.

So in the end my real question at this point is:

is it better to suppress the error message in QEMU (1) or implement an empty 
VHOST_USER_SEND_RARP in Passt (2)?

As this discussion is going on I'm no longer sure that (1) is the right one.

Thanks,
Laurent

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Daniel P. Berrangé]

On Wed, Jan 22, 2025 at 06:22:06PM +0100, Laurent Vivier wrote:
> On 22/01/2025 17:51, Stefano Garzarella wrote:
> > On Wed, Jan 22, 2025 at 05:41:15PM +0100, Laurent Vivier wrote:
> > > On 22/01/2025 17:20, Stefano Garzarella wrote:
> > > > On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
> > > > > On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
> > > > > > On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
> > > > > > > In vhost_user_receive() if vhost_net_notify_migration_done() reports
> > > > > > > an error we display on the console:
> > > > > > > 
> > > > > > >   Vhost user backend fails to broadcast fake RARP
> > > > > > > 
> > > > > > > This message can be useful if there is a problem to execute
> > > > > > > VHOST_USER_SEND_RARP but it is useless if the backend doesn't
> > > > > > > support VHOST_USER_PROTOCOL_F_RARP.
> > > > > > > 
> > > > > > > Don't report the error if vhost_net_notify_migration_done()
> > > > > > > returns -ENOTSUP (from vhost_user_migration_done())
> > > > > > > 
> > > > > > > Update vhost_net-stub.c to return -ENOTSUP too.
> > > > > > > 
> > > > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > > > > ---
> > > > > > > hw/net/vhost_net-stub.c | 2 +-
> > > > > > > net/vhost-user.c        | 2 +-
> > > > > > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
> > > > > > > index 72df6d757e4d..875cd6c2b9c8 100644
> > > > > > > --- a/hw/net/vhost_net-stub.c
> > > > > > > +++ b/hw/net/vhost_net-stub.c
> > > > > > > @@ -93,7 +93,7 @@ void
> > > > > > > vhost_net_config_mask(VHostNetState
> > > > > > *net, VirtIODevice *dev, bool mask)
> > > > > > > 
> > > > > > > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
> > > > > > > {
> > > > > > > -    return -1;
> > > > > > > +    return -ENOTSUP;
> > > > > > > }
> > > > > > > 
> > > > > > > VHostNetState *get_vhost_net(NetClientState *nc)
> > > > > > > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > > > > > > index 12555518e838..636fff8a84a2 100644
> > > > > > > --- a/net/vhost-user.c
> > > > > > > +++ b/net/vhost-user.c
> > > > > > > @@ -146,7 +146,7 @@ static ssize_t
> > > > > > vhost_user_receive(NetClientState *nc, const uint8_t *buf,
> > > > > > > 
> > > > > > >          r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
> > > > > > > 
> > > > > > > -        if ((r != 0) && (display_rarp_failure)) {
> > > > > > > +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
> > > > > > >              fprintf(stderr,
> > > > > > >                      "Vhost user backend fails to broadcast fake RARP\n");
> > > > > > >              fflush(stderr);
> > > > > > > -- 
> > > > > > > 2.47.1
> > > > > > > 
> > > > > > 
> > > > > > IIUC the message was there since the introduction about 10 years ago
> > > > > > from commit 3e866365e1 ("vhost user: add rarp sending after live
> > > > > > migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
> > > > > > and F_GUEST_ANNOUNCE are not negotiated.
> > > > > > 
> > > > > > That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
> > > > > 
> > > > > rarp is to have destination host broadcast a message with VM address
> > > > > to update the network. Guest announce is when it will instead
> > > > > ask the guest to do this.
> > > > 
> > > > Okay, thanks for explaining to me.
> > > > So if both features are not negotiated, no one is going to broadcast
> > > > the message, right?
> > > > 
> > > > Could that be a valid reason to print an error message in QEMU?
> > > > 
> > > > To me it might be reasonable because the user might experience some
> > > > network problems, but I'm not a network guy :-)
> > > 
> > > I'm working on adding vhost-user to passt[1], and in this case we
> > > don't need to broadcast any message.
> > 
> > Okay, so please can you add that to the commit description and also
> > explaining why you don't need that?
> > 
> > > 
> > > So I don't implement VHOST_USER_SEND_RARP and I don't want the error
> > > message to spoil my console.
> > 
> > Fair enough, but at that point, if it's valid to have both feature not
> > negotiated, IMHO is better to return 0 in vhost_user_migration_done().
> > Maybe adding also a comment to explain that in your scenario you don't
> > need to do nothing (like if guest supports GUEST_ANNOUNCE).
> 
> I agree.
> 
> > 
> > > 
> > > -ENOTSUP is an error message for developer not for user.
> > 
> > I was referring to the "Vhost user backend fails to broadcast fake RARP"
> > error message we are skipping here.
> 
> So in the end my real question at this point is:
> 
> is it better to suppress the error message in QEMU (1) or implement an empty
> VHOST_USER_SEND_RARP in Passt (2)?

If it is valid to interpret "VHOST_USER_SEND_RARP" to mean the service
is "capable of sending a RARP where appropriate" rather than "will always
unconditionally send an RARP", then it would be reasonable for Passt to
have an empty VHOST_USER_SEND_RARP impl.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Stefano Brivio]

[Cc'ed Thibaut as author of 3e866365e1eb ("vhost user: add rarp sending
after live migration for legacy guest")]

On Wed, 22 Jan 2025 17:51:07 +0100
Stefano Garzarella <sgarzare@redhat.com> wrote:

> On Wed, Jan 22, 2025 at 05:41:15PM +0100, Laurent Vivier wrote:
> >On 22/01/2025 17:20, Stefano Garzarella wrote:  
> >>On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:  
> >>>On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:  
> >>>>On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:  
> >>>>> In vhost_user_receive() if vhost_net_notify_migration_done() reports
> >>>>> an error we display on the console:
> >>>>>
> >>>>>  Vhost user backend fails to broadcast fake RARP
> >>>>>
> >>>>> This message can be useful if there is a problem to execute
> >>>>> VHOST_USER_SEND_RARP but it is useless if the backend doesn't
> >>>>> support VHOST_USER_PROTOCOL_F_RARP.
> >>>>>
> >>>>> Don't report the error if vhost_net_notify_migration_done()
> >>>>> returns -ENOTSUP (from vhost_user_migration_done())
> >>>>>
> >>>>> Update vhost_net-stub.c to return -ENOTSUP too.
> >>>>>
> >>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>>> ---
> >>>>> hw/net/vhost_net-stub.c | 2 +-
> >>>>> net/vhost-user.c        | 2 +-
> >>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
> >>>>> index 72df6d757e4d..875cd6c2b9c8 100644
> >>>>> --- a/hw/net/vhost_net-stub.c
> >>>>> +++ b/hw/net/vhost_net-stub.c
> >>>>> @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState   
> >>>>*net, VirtIODevice *dev, bool mask)  
> >>>>>
> >>>>> int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
> >>>>> {
> >>>>> -    return -1;
> >>>>> +    return -ENOTSUP;
> >>>>> }
> >>>>>
> >>>>> VHostNetState *get_vhost_net(NetClientState *nc)
> >>>>> diff --git a/net/vhost-user.c b/net/vhost-user.c
> >>>>> index 12555518e838..636fff8a84a2 100644
> >>>>> --- a/net/vhost-user.c
> >>>>> +++ b/net/vhost-user.c
> >>>>> @@ -146,7 +146,7 @@ static ssize_t   
> >>>>vhost_user_receive(NetClientState *nc, const uint8_t *buf,  
> >>>>>
> >>>>>         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
> >>>>>
> >>>>> -        if ((r != 0) && (display_rarp_failure)) {
> >>>>> +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
> >>>>>             fprintf(stderr,
> >>>>>                     "Vhost user backend fails to broadcast fake RARP\n");
> >>>>>             fflush(stderr);
> >>>>> --
> >>>>> 2.47.1
> >>>>>  
> >>>>
> >>>>IIUC the message was there since the introduction about 10 years ago
> >>>>from commit 3e866365e1 ("vhost user: add rarp sending after live
> >>>>migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
> >>>>and F_GUEST_ANNOUNCE are not negotiated.
> >>>>
> >>>>That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,  
> >>>
> >>>rarp is to have destination host broadcast a message with VM address
> >>>to update the network. Guest announce is when it will instead
> >>>ask the guest to do this.  
> >>
> >>Okay, thanks for explaining to me.
> >>So if both features are not negotiated, no one is going to broadcast
> >>the message, right?
> >>
> >>Could that be a valid reason to print an error message in QEMU?
> >>
> >>To me it might be reasonable because the user might experience some
> >>network problems, but I'm not a network guy :-)  
> >
> >I'm working on adding vhost-user to passt[1], and in this case we 
> >don't need to broadcast any message.  
> 
> Okay, so please can you add that to the commit description and also
> explaining why you don't need that?

By the way, we don't need that in passt because we don't need to update
any ARP table. Even if the guest changes its MAC address, with passt,
nobody is going to notice.

> >So I don't implement VHOST_USER_SEND_RARP and I don't want the error 
> >message to spoil my console.  
> 
> Fair enough, but at that point, if it's valid to have both feature not
> negotiated, IMHO is better to return 0 in vhost_user_migration_done().
> Maybe adding also a comment to explain that in your scenario you don't
> need to do nothing (like if guest supports GUEST_ANNOUNCE).
> 
> >-ENOTSUP is an error message for developer not for user.  
> 
> I was referring to the "Vhost user backend fails to broadcast fake RARP"
> error message we are skipping here.

By the way, I think that that message is inaccurate on a number of
other levels:

- if the back-end doesn't even try, it can't fail, and "fails"
  indicates... failure?

- RARP is an obsolete protocol, but there's no such thing as a fake
  protocol. It's not even a fake message, it's a simple RARP broadcast
  message, with the correct MAC address. It's "dummy", at most

- I read the whole thread but still I can't understand why we have a
  capability then: what does it (or its absence) mean?

Regardless of that, sure, let's go ahead and add the fake (that one
*is* fake) callback in passt, which Laurent just sent a patch for. We
need it anyway as a workaround for compatibility with current/older
versions of QEMU.

But I don't understand why we're leaving this as it is.

-- 
Stefano

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Michael S. Tsirkin]

On Fri, Jan 24, 2025 at 05:03:27PM +0100, Stefano Brivio wrote:
> But I don't understand why we're leaving this as it is.


So that people notice if there's some backend problem and
announcements are not going out. should help debug migration
issues. which we had, so we added this :)

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Stefano Brivio]

On Thu, 20 Feb 2025 10:28:20 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 24, 2025 at 05:03:27PM +0100, Stefano Brivio wrote:
> > But I don't understand why we're leaving this as it is.  
> 
> So that people notice if there's some backend problem and
> announcements are not going out. should help debug migration
> issues. which we had, so we added this :)

The message mentions that the back-end fails to do something it didn't
and can't even do, that's (one reason) why it's wrong (and confusing)
and this patch is obviously correct.

Perhaps the commit title isn't entirely accurate (it should say "when
unsupported", I guess) but it's somewhat expected to sacrifice detail
in the name of brevity, there. A glimpse at the message is enough.

Laurent now added a workaround in passt to pretend that we support
VHOST_USER_PROTOCOL_F_RARP by doing nothing in the callback, report
success, and silence the warning:

  https://passt.top/passt/commit/?id=dd6a6854c73a09c4091c1776ee7f349d1e1f966c

but having to do this kind of stuff is a bit unexpected while
interacting with another opensource project.

-- 
Stefano

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Michael S. Tsirkin]

On Thu, Feb 20, 2025 at 05:59:10PM +0100, Stefano Brivio wrote:
> On Thu, 20 Feb 2025 10:28:20 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jan 24, 2025 at 05:03:27PM +0100, Stefano Brivio wrote:
> > > But I don't understand why we're leaving this as it is.  
> > 
> > So that people notice if there's some backend problem and
> > announcements are not going out. should help debug migration
> > issues. which we had, so we added this :)
> 
> The message mentions that the back-end fails to do something it didn't
> and can't even do, that's (one reason) why it's wrong (and confusing)
> and this patch is obviously correct.
> 
> Perhaps the commit title isn't entirely accurate (it should say "when
> unsupported", I guess) but it's somewhat expected to sacrifice detail
> in the name of brevity, there. A glimpse at the message is enough.
> 
> Laurent now added a workaround in passt to pretend that we support
> VHOST_USER_PROTOCOL_F_RARP by doing nothing in the callback, report
> success, and silence the warning:
> 
>   https://passt.top/passt/commit/?id=dd6a6854c73a09c4091c1776ee7f349d1e1f966c
> 
> but having to do this kind of stuff is a bit unexpected while
> interacting with another opensource project.
> 
> -- 
> Stefano


let me explain. historically backends did not support migration.
then migration was added. as it was assumed RARP is required,
we did not add a feature flag for "supports migration" and
instead just assumed that VHOST_USER_PROTOCOL_F_RARP is that.

If you silence the warning you silence it for old backends
with no migration support.
If you want a new flag "migration with no RARP", be my
guest and add it.
Or if you want to add documentation explaining the meaning
better and clarifying the message.

-- 
MST

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Stefano Brivio]

On Thu, 20 Feb 2025 13:21:33 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 20, 2025 at 05:59:10PM +0100, Stefano Brivio wrote:
> > On Thu, 20 Feb 2025 10:28:20 -0500
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Fri, Jan 24, 2025 at 05:03:27PM +0100, Stefano Brivio wrote:  
> > > > But I don't understand why we're leaving this as it is.    
> > > 
> > > So that people notice if there's some backend problem and
> > > announcements are not going out. should help debug migration
> > > issues. which we had, so we added this :)  
> > 
> > The message mentions that the back-end fails to do something it didn't
> > and can't even do, that's (one reason) why it's wrong (and confusing)
> > and this patch is obviously correct.
> > 
> > Perhaps the commit title isn't entirely accurate (it should say "when
> > unsupported", I guess) but it's somewhat expected to sacrifice detail
> > in the name of brevity, there. A glimpse at the message is enough.
> > 
> > Laurent now added a workaround in passt to pretend that we support
> > VHOST_USER_PROTOCOL_F_RARP by doing nothing in the callback, report
> > success, and silence the warning:
> > 
> >   https://passt.top/passt/commit/?id=dd6a6854c73a09c4091c1776ee7f349d1e1f966c
> > 
> > but having to do this kind of stuff is a bit unexpected while
> > interacting with another opensource project.
> > 
> > -- 
> > Stefano  
> 
> 
> let me explain. historically backends did not support migration.
> then migration was added. as it was assumed RARP is required,
> we did not add a feature flag for "supports migration" and
> instead just assumed that VHOST_USER_PROTOCOL_F_RARP is that.
> 
> If you silence the warning you silence it for old backends
> with no migration support.

Thanks for the explanation. I'm struggling to grasp this. So if a
back-end doesn't support migration, because VHOST_USER_PROTOCOL_F_RARP
is not present in the features flag, migration is done anyway, but then
this is printed:

  Vhost user backend fails to broadcast fake RARP

with the meaning of:

  We did migration even if the back-end doesn't support it, whoops

?

Note that the message is printed *after* the migration and the flag is
*not* checked before.

> If you want a new flag "migration with no RARP", be my
> guest and add it.

That would actually make more sense than the existing situation I
think. VHOST_USER_PROTOCOL_F_NO_RARP?

I didn't understand, yet, what the exact meaning would be, though.

> Or if you want to add documentation explaining the meaning
> better and clarifying the message.

I'm still in the phase where I'm trying to understand the role of the
message :) ...I have to say this is fairly different now from what was
mentioned on the thread so far.

-- 
Stefano

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Michael S. Tsirkin]

On Thu, Feb 20, 2025 at 09:00:04PM +0100, Stefano Brivio wrote:
> On Thu, 20 Feb 2025 13:21:33 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Feb 20, 2025 at 05:59:10PM +0100, Stefano Brivio wrote:
> > > On Thu, 20 Feb 2025 10:28:20 -0500
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Fri, Jan 24, 2025 at 05:03:27PM +0100, Stefano Brivio wrote:  
> > > > > But I don't understand why we're leaving this as it is.    
> > > > 
> > > > So that people notice if there's some backend problem and
> > > > announcements are not going out. should help debug migration
> > > > issues. which we had, so we added this :)  
> > > 
> > > The message mentions that the back-end fails to do something it didn't
> > > and can't even do, that's (one reason) why it's wrong (and confusing)
> > > and this patch is obviously correct.
> > > 
> > > Perhaps the commit title isn't entirely accurate (it should say "when
> > > unsupported", I guess) but it's somewhat expected to sacrifice detail
> > > in the name of brevity, there. A glimpse at the message is enough.
> > > 
> > > Laurent now added a workaround in passt to pretend that we support
> > > VHOST_USER_PROTOCOL_F_RARP by doing nothing in the callback, report
> > > success, and silence the warning:
> > > 
> > >   https://passt.top/passt/commit/?id=dd6a6854c73a09c4091c1776ee7f349d1e1f966c
> > > 
> > > but having to do this kind of stuff is a bit unexpected while
> > > interacting with another opensource project.
> > > 
> > > -- 
> > > Stefano  
> > 
> > 
> > let me explain. historically backends did not support migration.
> > then migration was added. as it was assumed RARP is required,
> > we did not add a feature flag for "supports migration" and
> > instead just assumed that VHOST_USER_PROTOCOL_F_RARP is that.
> > 
> > If you silence the warning you silence it for old backends
> > with no migration support.
> 
> Thanks for the explanation. I'm struggling to grasp this. So if a
> back-end doesn't support migration, because VHOST_USER_PROTOCOL_F_RARP
> is not present in the features flag, migration is done anyway, but then
> this is printed:
> 
>   Vhost user backend fails to broadcast fake RARP
> 
> with the meaning of:
> 
>   We did migration even if the back-end doesn't support it, whoops
> 
> ?
> 
> Note that the message is printed *after* the migration and the flag is
> *not* checked before.
> 
> > If you want a new flag "migration with no RARP", be my
> > guest and add it.
> 
> That would actually make more sense than the existing situation I
> think. VHOST_USER_PROTOCOL_F_NO_RARP?
> 
> I didn't understand, yet, what the exact meaning would be, though.
> 
> > Or if you want to add documentation explaining the meaning
> > better and clarifying the message.
> 
> I'm still in the phase where I'm trying to understand the role of the
> message :) ...I have to say this is fairly different now from what was
> mentioned on the thread so far.

I'm going by memory. We made it a warning on the assumption that hey,
maybe someone has a way to migrate without a RARP, let them work.
Exactly what happened, we just did not think it through completely :)


-- 
MST

Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Jason Wang]

On Fri, Feb 21, 2025 at 4:46 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Feb 20, 2025 at 09:00:04PM +0100, Stefano Brivio wrote:
> > On Thu, 20 Feb 2025 13:21:33 -0500
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Thu, Feb 20, 2025 at 05:59:10PM +0100, Stefano Brivio wrote:
> > > > On Thu, 20 Feb 2025 10:28:20 -0500
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Fri, Jan 24, 2025 at 05:03:27PM +0100, Stefano Brivio wrote:
> > > > > > But I don't understand why we're leaving this as it is.
> > > > >
> > > > > So that people notice if there's some backend problem and
> > > > > announcements are not going out. should help debug migration
> > > > > issues. which we had, so we added this :)
> > > >
> > > > The message mentions that the back-end fails to do something it didn't
> > > > and can't even do, that's (one reason) why it's wrong (and confusing)
> > > > and this patch is obviously correct.
> > > >
> > > > Perhaps the commit title isn't entirely accurate (it should say "when
> > > > unsupported", I guess) but it's somewhat expected to sacrifice detail
> > > > in the name of brevity, there. A glimpse at the message is enough.
> > > >
> > > > Laurent now added a workaround in passt to pretend that we support
> > > > VHOST_USER_PROTOCOL_F_RARP by doing nothing in the callback, report
> > > > success, and silence the warning:
> > > >
> > > >   https://passt.top/passt/commit/?id=dd6a6854c73a09c4091c1776ee7f349d1e1f966c
> > > >
> > > > but having to do this kind of stuff is a bit unexpected while
> > > > interacting with another opensource project.
> > > >
> > > > --
> > > > Stefano
> > >
> > >
> > > let me explain. historically backends did not support migration.
> > > then migration was added. as it was assumed RARP is required,
> > > we did not add a feature flag for "supports migration" and
> > > instead just assumed that VHOST_USER_PROTOCOL_F_RARP is that.
> > >
> > > If you silence the warning you silence it for old backends
> > > with no migration support.
> >
> > Thanks for the explanation. I'm struggling to grasp this. So if a
> > back-end doesn't support migration, because VHOST_USER_PROTOCOL_F_RARP
> > is not present in the features flag, migration is done anyway, but then
> > this is printed:
> >
> >   Vhost user backend fails to broadcast fake RARP
> >
> > with the meaning of:
> >
> >   We did migration even if the back-end doesn't support it, whoops
> >
> > ?
> >
> > Note that the message is printed *after* the migration and the flag is
> > *not* checked before.
> >
> > > If you want a new flag "migration with no RARP", be my
> > > guest and add it.
> >
> > That would actually make more sense than the existing situation I
> > think. VHOST_USER_PROTOCOL_F_NO_RARP?
> >
> > I didn't understand, yet, what the exact meaning would be, though.
> >
> > > Or if you want to add documentation explaining the meaning
> > > better and clarifying the message.
> >
> > I'm still in the phase where I'm trying to understand the role of the
> > message :) ...I have to say this is fairly different now from what was
> > mentioned on the thread so far.
>
> I'm going by memory. We made it a warning on the assumption that hey,
> maybe someone has a way to migrate without a RARP, let them work.

Migration still works, it might just increase the time spent on
recovering the networking topology by switching if the guest doesn't
send anything.

Or I've heard from the upper layer team that the control/management
plane may be in charge of the network recovery after migration so RARP
is not a must.

> Exactly what happened, we just did not think it through completely :)

Thanks

>
>
> --
> MST
>