[PATCH] ndp: Don't send first periodic router advertisement right after guest connects

[PATCH] ndp: Don't send first periodic router advertisement right after guest connects

[Stefano Brivio]

This is very visible with muvm, but it also happens with QEMU: we're
sending the first unsolicited router advertisement milliseconds after
the guest connects.

That's usually pointless because, when the hypervisor connects, the
guest is typically not ready yet to process anything of that sort:
it's still booting. And if we happen to send it late enough (still
milliseconds), with muvm, while the message is discarded, it
sometimes (slightly) delays the response to the first solicited
router advertisement, which is the one we need to have coming fast.

Skip sending the unsolicited advertisement on the first timer run,
just calculate the next delay. Keep it simple by observing that we're
probably not trying to reach the 1970s with IPv6.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 ndp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ndp.c b/ndp.c
index 1752d64..37bf7a3 100644
--- a/ndp.c
+++ b/ndp.c
@@ -420,9 +420,13 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
 	interval = min_rtr_adv_interval +
 		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
+	if (!next_ra)
+		goto first;
+
 	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
 
 	ndp_ra(c, &in6addr_ll_all_nodes);
 
+first:
 	next_ra = now->tv_sec + interval;
 }
-- 
@@ -420,9 +420,13 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
 	interval = min_rtr_adv_interval +
 		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
+	if (!next_ra)
+		goto first;
+
 	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
 
 	ndp_ra(c, &in6addr_ll_all_nodes);
 
+first:
 	next_ra = now->tv_sec + interval;
 }
-- 
2.43.0

Re: [PATCH] ndp: Don't send first periodic router advertisement right after guest connects

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Mon, Nov 25, 2024 at 09:09:05AM +0100, Stefano Brivio wrote:
> This is very visible with muvm, but it also happens with QEMU: we're
> sending the first unsolicited router advertisement milliseconds after
> the guest connects.
> 
> That's usually pointless because, when the hypervisor connects, the
> guest is typically not ready yet to process anything of that sort:
> it's still booting. And if we happen to send it late enough (still
> milliseconds), with muvm, while the message is discarded, it
> sometimes (slightly) delays the response to the first solicited
> router advertisement, which is the one we need to have coming fast.
> 
> Skip sending the unsolicited advertisement on the first timer run,
> just calculate the next delay. Keep it simple by observing that we're
> probably not trying to reach the 1970s with IPv6.

So, as I wrote it, I wasn't particularly happy with how we handled
timing the first announcement (what does next_ra==0 even mean against
the monotonic clock?).

I guess this addresses a practical problem, and is no less logical.
I'm not especially convinced it's any *more* logical than the current
behaviour either, though.  I guess it works on the fiction that the
link's always been there, the guest is just seeing it for the first
time.  Under that fiction there would have been RAs in the past, and
this works out the next one at a plausible interval based on that.

> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  ndp.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/ndp.c b/ndp.c
> index 1752d64..37bf7a3 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -420,9 +420,13 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
>  	interval = min_rtr_adv_interval +
>  		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
>  
> +	if (!next_ra)
> +		goto first;

I don't think avoiding re-indenting two lines is sufficient reason to
introduce yet another goto, though..

> +
>  	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
>  
>  	ndp_ra(c, &in6addr_ll_all_nodes);
>  
> +first:
>  	next_ra = now->tv_sec + interval;
>  }

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ndp: Don't send first periodic router advertisement right after guest connects

[Stefano Brivio]

On Tue, 26 Nov 2024 15:22:38 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 25, 2024 at 09:09:05AM +0100, Stefano Brivio wrote:
> >
> > [...]
> >
> > @@ -420,9 +420,13 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
> >  	interval = min_rtr_adv_interval +
> >  		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
> >  
> > +	if (!next_ra)
> > +		goto first;  
> 
> I don't think avoiding re-indenting two lines is sufficient reason to
> introduce yet another goto, though..
> 
> > +
> >  	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
> >  
> >  	ndp_ra(c, &in6addr_ll_all_nodes);
> >  
> > +first:
> >  	next_ra = now->tv_sec + interval;
> >  }  

At the cost of one additional line (and zero non-blank lines):

--
$ git diff --patch --stat
 ndp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/ndp.c b/ndp.c
index 37bf7a3..d1ba867 100644
--- a/ndp.c
+++ b/ndp.c
@@ -420,13 +420,12 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
        interval = min_rtr_adv_interval +
                random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
-       if (!next_ra)
-               goto first;
+       if (next_ra) {
+               info("NDP: sending unsolicited RA, next in %llds",
+                    (long long)interval);
 
-       info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
-
-       ndp_ra(c, &in6addr_ll_all_nodes);
+               ndp_ra(c, &in6addr_ll_all_nodes);
+       }
 
-first:
        next_ra = now->tv_sec + interval;
 }
--

we get:

- clarity about the fact that 'next_ra' happens to be 0 on the *first*
  run (the label says it)

- clarity about the fact that it's a special case (it's a goto)

- no wrapped lines

-- 
@@ -420,13 +420,12 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
        interval = min_rtr_adv_interval +
                random() % (max_rtr_adv_interval - min_rtr_adv_interval);
 
-       if (!next_ra)
-               goto first;
+       if (next_ra) {
+               info("NDP: sending unsolicited RA, next in %llds",
+                    (long long)interval);
 
-       info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
-
-       ndp_ra(c, &in6addr_ll_all_nodes);
+               ndp_ra(c, &in6addr_ll_all_nodes);
+       }
 
-first:
        next_ra = now->tv_sec + interval;
 }
--

we get:

- clarity about the fact that 'next_ra' happens to be 0 on the *first*
  run (the label says it)

- clarity about the fact that it's a special case (it's a goto)

- no wrapped lines

-- 
Stefano

Re: [PATCH] ndp: Don't send first periodic router advertisement right after guest connects

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On Tue, Nov 26, 2024 at 05:48:03AM +0100, Stefano Brivio wrote:
> On Tue, 26 Nov 2024 15:22:38 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Nov 25, 2024 at 09:09:05AM +0100, Stefano Brivio wrote:
> > >
> > > [...]
> > >
> > > @@ -420,9 +420,13 @@ void ndp_timer(const struct ctx *c, const struct timespec *now)
> > >  	interval = min_rtr_adv_interval +
> > >  		random() % (max_rtr_adv_interval - min_rtr_adv_interval);
> > >  
> > > +	if (!next_ra)
> > > +		goto first;  
> > 
> > I don't think avoiding re-indenting two lines is sufficient reason to
> > introduce yet another goto, though..
> > 
> > > +
> > >  	info("NDP: sending unsolicited RA, next in %llds", (long long)interval);
> > >  
> > >  	ndp_ra(c, &in6addr_ll_all_nodes);
> > >  
> > > +first:
> > >  	next_ra = now->tv_sec + interval;
> > >  }  
> 
> At the cost of one additional line (and zero non-blank lines):

Eh.. I'm not really convinced, but close enough that I can't be
bothered arguing it further.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]