[PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Jack Wang]

So it can handle more incoming requests.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 migration/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index c7c7a384875b..2e223170d06d 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
 
     trace_rdma_start_incoming_migration_after_dest_init();
 
-    ret = rdma_listen(rdma->listen_id, 5);
+    ret = rdma_listen(rdma->listen_id, 128);
 
     if (ret) {
         ERROR(errp, "listening on socket!");
-- 
2.25.1

[PATCH 2/2] migration/rdma: set the REUSEADDR option for destination

[Jack Wang]

This allow address could be reused to avoid rdma_bind_addr error
out.

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 migration/rdma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration/rdma.c b/migration/rdma.c
index 2e223170d06d..b498ef013c77 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
     char ip[40] = "unknown";
     struct rdma_addrinfo *res, *e;
     char port_str[16];
+    int reuse = 1;
 
     for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
         rdma->wr_data[idx].control_len = 0;
@@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
         goto err_dest_init_bind_addr;
     }
 
+    ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR,
+			  &reuse, sizeof reuse);
+    if (ret) {
+        ERROR(errp, "Error: could not set REUSEADDR option");
+        goto err_dest_init_bind_addr;
+    }
     for (e = res; e != NULL; e = e->ai_next) {
         inet_ntop(e->ai_family,
             &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
-- 
2.25.1

Re: [PATCH 2/2] migration/rdma: set the REUSEADDR option for destination

[Pankaj Gupta]

> This allow address could be reused to avoid rdma_bind_addr error
> out.

Seems we are proposing to allow multiple connections on same source ip
port pair?
>
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  migration/rdma.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 2e223170d06d..b498ef013c77 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>      char ip[40] = "unknown";
>      struct rdma_addrinfo *res, *e;
>      char port_str[16];
> +    int reuse = 1;
>
>      for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
>          rdma->wr_data[idx].control_len = 0;
> @@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>          goto err_dest_init_bind_addr;
>      }
>
> +    ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR,
> +                         &reuse, sizeof reuse);

maybe we can just write '1' directly on the argument list of 'rdma_set_option'.
Assuming reuseaddr does not effect core rdma transport? change seems ok to me.

Thanks,
Pankaj

> +    if (ret) {
> +        ERROR(errp, "Error: could not set REUSEADDR option");
> +        goto err_dest_init_bind_addr;
> +    }
>      for (e = res; e != NULL; e = e->ai_next) {
>          inet_ntop(e->ai_family,
>              &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> --
> 2.25.1
>

Re: [PATCH 2/2] migration/rdma: set the REUSEADDR option for destination

[Jinpu Wang]

On Tue, Feb 1, 2022 at 7:39 PM Pankaj Gupta <pankaj.gupta@ionos.com> wrote:
>
> > This allow address could be reused to avoid rdma_bind_addr error
> > out.
>
> Seems we are proposing to allow multiple connections on same source ip
> port pair?
according to the man page, it's more about the destination side which
is the incoming side.[1]
We hit the error on the migration target when there are many migration
tests in parallel:
"RDMA ERROR: Error: could not rdma_bind_addr!"

[1]https://manpages.debian.org/testing/librdmacm-dev/rdma_set_option.3.en.html
> >
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  migration/rdma.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 2e223170d06d..b498ef013c77 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
> >      char ip[40] = "unknown";
> >      struct rdma_addrinfo *res, *e;
> >      char port_str[16];
> > +    int reuse = 1;
> >
> >      for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
> >          rdma->wr_data[idx].control_len = 0;
> > @@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
> >          goto err_dest_init_bind_addr;
> >      }
> >
> > +    ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR,
> > +                         &reuse, sizeof reuse);
>
> maybe we can just write '1' directly on the argument list of 'rdma_set_option'.
> Assuming reuseaddr does not effect core rdma transport? change seems ok to me.
I feel it's cleaner to do it with a variable than force conversion of
1 to void *.

It's bound to the cm_id which is newly created a few lines above, so
does not affect core rdma transport.

>
> Thanks,
> Pankaj
Thanks for the review!

Jinpu Wang
>
> > +    if (ret) {
> > +        ERROR(errp, "Error: could not set REUSEADDR option");
> > +        goto err_dest_init_bind_addr;
> > +    }
> >      for (e = res; e != NULL; e = e->ai_next) {
> >          inet_ntop(e->ai_family,
> >              &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> > --
> > 2.25.1
> >

Re: [PATCH 2/2] migration/rdma: set the REUSEADDR option for destination

[Pankaj Gupta]

> > > This allow address could be reused to avoid rdma_bind_addr error
> > > out.
> >
> > Seems we are proposing to allow multiple connections on same source ip
> > port pair?
> according to the man page, it's more about the destination side which
> is the incoming side.[1]

By source here I meant generic ip port address pair binding. For this case it is
at destination live migration host.

> We hit the error on the migration target when there are many migration
> tests in parallel:
> "RDMA ERROR: Error: could not rdma_bind_addr!"
o.k. Worth to add this in commit message.

>
> [1]https://manpages.debian.org/testing/librdmacm-dev/rdma_set_option.3.en.html
> > >
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > ---
> > >  migration/rdma.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index 2e223170d06d..b498ef013c77 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
> > >      char ip[40] = "unknown";
> > >      struct rdma_addrinfo *res, *e;
> > >      char port_str[16];
> > > +    int reuse = 1;
> > >
> > >      for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
> > >          rdma->wr_data[idx].control_len = 0;
> > > @@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
> > >          goto err_dest_init_bind_addr;
> > >      }
> > >
> > > +    ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR,
> > > +                         &reuse, sizeof reuse);
> >
> > maybe we can just write '1' directly on the argument list of 'rdma_set_option'.
> > Assuming reuseaddr does not effect core rdma transport? change seems ok to me.
> I feel it's cleaner to do it with a variable than force conversion of
> 1 to void *.

Adding typecasted 1 seems more cleaner than adding another auto variable.
Will leave this to maintainers to decide.
>
> It's bound to the cm_id which is newly created a few lines above, so
> does not affect core rdma transport.

o.k.
>
> >
> > Thanks,
> > Pankaj
> Thanks for the review!
>
> Jinpu Wang
> >
> > > +    if (ret) {
> > > +        ERROR(errp, "Error: could not set REUSEADDR option");
> > > +        goto err_dest_init_bind_addr;
> > > +    }
> > >      for (e = res; e != NULL; e = e->ai_next) {
> > >          inet_ntop(e->ai_family,
> > >              &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> > > --
> > > 2.25.1
> > >

Re: [PATCH 2/2] migration/rdma: set the REUSEADDR option for destination

[Dr. David Alan Gilbert]

* Jack Wang (jinpu.wang@ionos.com) wrote:
> This allow address could be reused to avoid rdma_bind_addr error
> out.

In what case do you get the error - after a failed migrate and then a
retry?

Dave

> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  migration/rdma.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 2e223170d06d..b498ef013c77 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>      char ip[40] = "unknown";
>      struct rdma_addrinfo *res, *e;
>      char port_str[16];
> +    int reuse = 1;
>  
>      for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
>          rdma->wr_data[idx].control_len = 0;
> @@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
>          goto err_dest_init_bind_addr;
>      }
>  
> +    ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR,
> +			  &reuse, sizeof reuse);
> +    if (ret) {
> +        ERROR(errp, "Error: could not set REUSEADDR option");
> +        goto err_dest_init_bind_addr;
> +    }
>      for (e = res; e != NULL; e = e->ai_next) {
>          inet_ntop(e->ai_family,
>              &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/2] migration/rdma: set the REUSEADDR option for destination

[Jinpu Wang]

On Wed, Feb 2, 2022 at 11:15 AM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Jack Wang (jinpu.wang@ionos.com) wrote:
> > This allow address could be reused to avoid rdma_bind_addr error
> > out.
>
> In what case do you get the error - after a failed migrate and then a
> retry?

Yes, what I saw is in case of error, mgmt daemon pick one migration port,
incoming rdma:[::]:8089: RDMA ERROR: Error: could not rdma_bind_addr

Then try another -incoming rdma:[::]:8103, sometime it worked,
sometimes need another try with other ports number.

with this patch, I don't see the error anymore.
>
> Dave
Thanks!
>
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  migration/rdma.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 2e223170d06d..b498ef013c77 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -2705,6 +2705,7 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
> >      char ip[40] = "unknown";
> >      struct rdma_addrinfo *res, *e;
> >      char port_str[16];
> > +    int reuse = 1;
> >
> >      for (idx = 0; idx < RDMA_WRID_MAX; idx++) {
> >          rdma->wr_data[idx].control_len = 0;
> > @@ -2740,6 +2741,12 @@ static int qemu_rdma_dest_init(RDMAContext *rdma, Error **errp)
> >          goto err_dest_init_bind_addr;
> >      }
> >
> > +    ret = rdma_set_option(listen_id, RDMA_OPTION_ID, RDMA_OPTION_ID_REUSEADDR,
> > +                       &reuse, sizeof reuse);
> > +    if (ret) {
> > +        ERROR(errp, "Error: could not set REUSEADDR option");
> > +        goto err_dest_init_bind_addr;
> > +    }
> >      for (e = res; e != NULL; e = e->ai_next) {
> >          inet_ntop(e->ai_family,
> >              &((struct sockaddr_in *) e->ai_dst_addr)->sin_addr, ip, sizeof ip);
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Pankaj Gupta]

> So it can handle more incoming requests.
>
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  migration/rdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index c7c7a384875b..2e223170d06d 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
>
>      trace_rdma_start_incoming_migration_after_dest_init();
>
> -    ret = rdma_listen(rdma->listen_id, 5);
> +    ret = rdma_listen(rdma->listen_id, 128);

128 backlog seems too much to me. Any reason for choosing this number.
Any rationale to choose this number?

Thanks,
Pankaj

>
>      if (ret) {
>          ERROR(errp, "listening on socket!");
> --
> 2.25.1
>

Re: [PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Jinpu Wang]

On Tue, Feb 1, 2022 at 7:19 PM Pankaj Gupta <pankaj.gupta@ionos.com> wrote:
>
> > So it can handle more incoming requests.
> >
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  migration/rdma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index c7c7a384875b..2e223170d06d 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
> >
> >      trace_rdma_start_incoming_migration_after_dest_init();
> >
> > -    ret = rdma_listen(rdma->listen_id, 5);
> > +    ret = rdma_listen(rdma->listen_id, 128);
>
> 128 backlog seems too much to me. Any reason for choosing this number.
> Any rationale to choose this number?
>
128 is the default value of SOMAXCONN, I can use that if it is preferred.

> Thanks,
> Pankaj

Thanks!
>
> >
> >      if (ret) {
> >          ERROR(errp, "listening on socket!");
> > --
> > 2.25.1
> >

Re: [PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Pankaj Gupta]

> > > So it can handle more incoming requests.
> > >
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > ---
> > >  migration/rdma.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index c7c7a384875b..2e223170d06d 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
> > >
> > >      trace_rdma_start_incoming_migration_after_dest_init();
> > >
> > > -    ret = rdma_listen(rdma->listen_id, 5);
> > > +    ret = rdma_listen(rdma->listen_id, 128);
> >
> > 128 backlog seems too much to me. Any reason for choosing this number.
> > Any rationale to choose this number?
> >
> 128 is the default value of SOMAXCONN, I can use that if it is preferred.

AFAICS backlog is only applicable with RDMA iWARP CM mode. Maybe we
can increase it to 128.
Maybe you can also share any testing data for multiple concurrent live
migrations using RDMA, please.

Thanks,
Pankaj

Thanks,
Pankaj

Re: [PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Pankaj Gupta]

> > > >  migration/rdma.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > > index c7c7a384875b..2e223170d06d 100644
> > > > --- a/migration/rdma.c
> > > > +++ b/migration/rdma.c
> > > > @@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
> > > >
> > > >      trace_rdma_start_incoming_migration_after_dest_init();
> > > >
> > > > -    ret = rdma_listen(rdma->listen_id, 5);
> > > > +    ret = rdma_listen(rdma->listen_id, 128);
> > >
> > > 128 backlog seems too much to me. Any reason for choosing this number.
> > > Any rationale to choose this number?
> > >
> > 128 is the default value of SOMAXCONN, I can use that if it is preferred.
>
> AFAICS backlog is only applicable with RDMA iWARP CM mode. Maybe we
> can increase it to 128.these many

Or maybe we first increase it to 20 or 32? or so to avoid memory
overhead if we are not
using these many connections at the same time.

> Maybe you can also share any testing data for multiple concurrent live
> migrations using RDMA, please.
>
> Thanks,
> Pankaj
>
> Thanks,
> Pankaj

Re: [PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Dr. David Alan Gilbert]

* Pankaj Gupta (pankaj.gupta@ionos.com) wrote:
> > > > >  migration/rdma.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > > > index c7c7a384875b..2e223170d06d 100644
> > > > > --- a/migration/rdma.c
> > > > > +++ b/migration/rdma.c
> > > > > @@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
> > > > >
> > > > >      trace_rdma_start_incoming_migration_after_dest_init();
> > > > >
> > > > > -    ret = rdma_listen(rdma->listen_id, 5);
> > > > > +    ret = rdma_listen(rdma->listen_id, 128);
> > > >
> > > > 128 backlog seems too much to me. Any reason for choosing this number.
> > > > Any rationale to choose this number?
> > > >
> > > 128 is the default value of SOMAXCONN, I can use that if it is preferred.
> >
> > AFAICS backlog is only applicable with RDMA iWARP CM mode. Maybe we
> > can increase it to 128.these many
> 
> Or maybe we first increase it to 20 or 32? or so to avoid memory
> overhead if we are not
> using these many connections at the same time.

Can you explain why you're requiring more than 1?  Is this with multifd
patches?

Dave

> > Maybe you can also share any testing data for multiple concurrent live
> > migrations using RDMA, please.
> >
> > Thanks,
> > Pankaj
> >
> > Thanks,
> > Pankaj
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/2] migration/rdma: Increase the backlog from 5 to 128

[Jinpu Wang]

On Wed, Feb 2, 2022 at 10:20 AM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Pankaj Gupta (pankaj.gupta@ionos.com) wrote:
> > > > > >  migration/rdma.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > > > > index c7c7a384875b..2e223170d06d 100644
> > > > > > --- a/migration/rdma.c
> > > > > > +++ b/migration/rdma.c
> > > > > > @@ -4238,7 +4238,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp)
> > > > > >
> > > > > >      trace_rdma_start_incoming_migration_after_dest_init();
> > > > > >
> > > > > > -    ret = rdma_listen(rdma->listen_id, 5);
> > > > > > +    ret = rdma_listen(rdma->listen_id, 128);
> > > > >
> > > > > 128 backlog seems too much to me. Any reason for choosing this number.
> > > > > Any rationale to choose this number?
> > > > >
> > > > 128 is the default value of SOMAXCONN, I can use that if it is preferred.
> > >
> > > AFAICS backlog is only applicable with RDMA iWARP CM mode. Maybe we
> > > can increase it to 128.these many
> >
> > Or maybe we first increase it to 20 or 32? or so to avoid memory
> > overhead if we are not
> > using these many connections at the same time.
>
> Can you explain why you're requiring more than 1?  Is this with multifd
> patches?

no, I'm not using multifs patches, just code reading, I feel 5 is too
small for the backlog setting.

As Pankaj rightly mentioned, in RDMA, only iWARP CM will take some
effect, it does nothing for InfiniBand
and RoCE.

Please ignore this patch, we can revisit this when we introduce
multifid with RDMA.

Thanks!
Jinpu Wang
>
> Dave
>
> > > Maybe you can also share any testing data for multiple concurrent live
> > > migrations using RDMA, please.
> > >
> > > Thanks,
> > > Pankaj
> > >
> > > Thanks,
> > > Pankaj
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>