Because qio_channel_rdma_writev and qio_channel_rdma_readv maybe invoked
by different threads concurrently, this patch removes unnecessary variables
len in QIOChannelRDMA and use local variable instead.
Signed-off-by: Lidong Chen <lidongchen@tencent.com>
---
migration/rdma.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/migration/rdma.c b/migration/rdma.c
index c745427..f5c1d02 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -404,7 +404,6 @@ struct QIOChannelRDMA {
QIOChannel parent;
RDMAContext *rdma;
QEMUFile *file;
- size_t len;
bool blocking; /* XXX we don't actually honour this yet */
};
@@ -2640,6 +2639,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
int ret;
ssize_t done = 0;
size_t i;
+ size_t len = 0;
CHECK_ERROR_STATE();
@@ -2659,10 +2659,10 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
while (remaining) {
RDMAControlHeader head;
- rioc->len = MIN(remaining, RDMA_SEND_INCREMENT);
- remaining -= rioc->len;
+ len = MIN(remaining, RDMA_SEND_INCREMENT);
+ remaining -= len;
- head.len = rioc->len;
+ head.len = len;
head.type = RDMA_CONTROL_QEMU_FILE;
ret = qemu_rdma_exchange_send(rdma, &head, data, NULL, NULL, NULL);
@@ -2672,8 +2672,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
return ret;
}
- data += rioc->len;
- done += rioc->len;
+ data += len;
+ done += len;
}
}
@@ -2768,8 +2768,7 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
}
}
}
- rioc->len = done;
- return rioc->len;
+ return done;
}
/*
--
1.8.3.1
* Lidong Chen (jemmy858585@gmail.com) wrote:
> Because qio_channel_rdma_writev and qio_channel_rdma_readv maybe invoked
> by different threads concurrently, this patch removes unnecessary variables
> len in QIOChannelRDMA and use local variable instead.
>
> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
I'm OK with this patch as is; but now you're making me worried that I
don't quite understand what thrads are accessing it at the same time; we
need to document/comment what's accessed concurrently.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/rdma.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index c745427..f5c1d02 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -404,7 +404,6 @@ struct QIOChannelRDMA {
> QIOChannel parent;
> RDMAContext *rdma;
> QEMUFile *file;
> - size_t len;
> bool blocking; /* XXX we don't actually honour this yet */
> };
>
> @@ -2640,6 +2639,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
> int ret;
> ssize_t done = 0;
> size_t i;
> + size_t len = 0;
>
> CHECK_ERROR_STATE();
>
> @@ -2659,10 +2659,10 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
> while (remaining) {
> RDMAControlHeader head;
>
> - rioc->len = MIN(remaining, RDMA_SEND_INCREMENT);
> - remaining -= rioc->len;
> + len = MIN(remaining, RDMA_SEND_INCREMENT);
> + remaining -= len;
>
> - head.len = rioc->len;
> + head.len = len;
> head.type = RDMA_CONTROL_QEMU_FILE;
>
> ret = qemu_rdma_exchange_send(rdma, &head, data, NULL, NULL, NULL);
> @@ -2672,8 +2672,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
> return ret;
> }
>
> - data += rioc->len;
> - done += rioc->len;
> + data += len;
> + done += len;
> }
> }
>
> @@ -2768,8 +2768,7 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
> }
> }
> }
> - rioc->len = done;
> - return rioc->len;
> + return done;
> }
>
> /*
> --
> 1.8.3.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Apr 27, 2018 at 12:40 AM, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> * Lidong Chen (jemmy858585@gmail.com) wrote:
>> Because qio_channel_rdma_writev and qio_channel_rdma_readv maybe invoked
>> by different threads concurrently, this patch removes unnecessary variables
>> len in QIOChannelRDMA and use local variable instead.
>>
>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>
> I'm OK with this patch as is; but now you're making me worried that I
> don't quite understand what thrads are accessing it at the same time; we
> need to document/comment what's accessed concurrently..
for the source qemu, migration thread invokes qio_channel_rdma_writev,
and the return
path thread invokes qio_channel_rdma_readv.
for the destination qemu, before postcopy, the main thread invokes
qio_channel_rdma_readv
and qio_channel_rdma_writev.
after postcopy, the listen_thread invokes qio_channel_rdma_readv, and
the ram_fault_thread
invokes qio_channel_rdma_writev.
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> ---
>> migration/rdma.c | 15 +++++++--------
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index c745427..f5c1d02 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -404,7 +404,6 @@ struct QIOChannelRDMA {
>> QIOChannel parent;
>> RDMAContext *rdma;
>> QEMUFile *file;
>> - size_t len;
>> bool blocking; /* XXX we don't actually honour this yet */
>> };
>>
>> @@ -2640,6 +2639,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
>> int ret;
>> ssize_t done = 0;
>> size_t i;
>> + size_t len = 0;
>>
>> CHECK_ERROR_STATE();
>>
>> @@ -2659,10 +2659,10 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
>> while (remaining) {
>> RDMAControlHeader head;
>>
>> - rioc->len = MIN(remaining, RDMA_SEND_INCREMENT);
>> - remaining -= rioc->len;
>> + len = MIN(remaining, RDMA_SEND_INCREMENT);
>> + remaining -= len;
>>
>> - head.len = rioc->len;
>> + head.len = len;
>> head.type = RDMA_CONTROL_QEMU_FILE;
>>
>> ret = qemu_rdma_exchange_send(rdma, &head, data, NULL, NULL, NULL);
>> @@ -2672,8 +2672,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
>> return ret;
>> }
>>
>> - data += rioc->len;
>> - done += rioc->len;
>> + data += len;
>> + done += len;
>> }
>> }
>>
>> @@ -2768,8 +2768,7 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
>> }
>> }
>> }
>> - rioc->len = done;
>> - return rioc->len;
>> + return done;
>> }
>>
>> /*
>> --
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Apr 26, 2018 at 05:40:11PM +0100, Dr. David Alan Gilbert wrote:
> * Lidong Chen (jemmy858585@gmail.com) wrote:
> > Because qio_channel_rdma_writev and qio_channel_rdma_readv maybe invoked
> > by different threads concurrently, this patch removes unnecessary variables
> > len in QIOChannelRDMA and use local variable instead.
> >
> > Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>
> I'm OK with this patch as is; but now you're making me worried that I
> don't quite understand what thrads are accessing it at the same time; we
> need to document/comment what's accessed concurrently.
FWIW, I have always intended that QIOChannel impls be able to support
bidirectional I/O from multiple threads, provided they each are using
it in a different direction. So this is a mistake in my original RDMA
impl.
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> > ---
> > migration/rdma.c | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index c745427..f5c1d02 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -404,7 +404,6 @@ struct QIOChannelRDMA {
> > QIOChannel parent;
> > RDMAContext *rdma;
> > QEMUFile *file;
> > - size_t len;
> > bool blocking; /* XXX we don't actually honour this yet */
> > };
> >
> > @@ -2640,6 +2639,7 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
> > int ret;
> > ssize_t done = 0;
> > size_t i;
> > + size_t len = 0;
> >
> > CHECK_ERROR_STATE();
> >
> > @@ -2659,10 +2659,10 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
> > while (remaining) {
> > RDMAControlHeader head;
> >
> > - rioc->len = MIN(remaining, RDMA_SEND_INCREMENT);
> > - remaining -= rioc->len;
> > + len = MIN(remaining, RDMA_SEND_INCREMENT);
> > + remaining -= len;
> >
> > - head.len = rioc->len;
> > + head.len = len;
> > head.type = RDMA_CONTROL_QEMU_FILE;
> >
> > ret = qemu_rdma_exchange_send(rdma, &head, data, NULL, NULL, NULL);
> > @@ -2672,8 +2672,8 @@ static ssize_t qio_channel_rdma_writev(QIOChannel *ioc,
> > return ret;
> > }
> >
> > - data += rioc->len;
> > - done += rioc->len;
> > + data += len;
> > + done += len;
> > }
> > }
> >
> > @@ -2768,8 +2768,7 @@ static ssize_t qio_channel_rdma_readv(QIOChannel *ioc,
> > }
> > }
> > }
> > - rioc->len = done;
> > - return rioc->len;
> > + return done;
> > }
> >
> > /*
> > --
> > 1.8.3.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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 :|
On Wed, Apr 25, 2018 at 10:35:32PM +0800, Lidong Chen wrote: > Because qio_channel_rdma_writev and qio_channel_rdma_readv maybe invoked > by different threads concurrently, this patch removes unnecessary variables > len in QIOChannelRDMA and use local variable instead. > > Signed-off-by: Lidong Chen <lidongchen@tencent.com> > --- > migration/rdma.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
© 2016 - 2025 Red Hat, Inc.