From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Xiao Guangrong <guangrong.xiao@gmail.com> Cc: kvm@vger.kernel.org, mst@redhat.com, mtosatti@redhat.com, Xiao Guangrong <xiaoguangrong@tencent.com>, qemu-devel@nongnu.org, peterx@redhat.com, quintela@redhat.com, wei.w.wang@intel.com, cota@braap.org, jiang.biao2@zte.com.cn, pbonzini@redhat.com Subject: Re: [PATCH v3 2/5] util: introduce threaded workqueue Date: Mon, 26 Nov 2018 10:56:22 +0000 [thread overview] Message-ID: <20181126105622.GB2547@work-vm> (raw) In-Reply-To: <e60f648f-d475-a88b-b0c3-300f18a7d36d@gmail.com> * Xiao Guangrong (guangrong.xiao@gmail.com) wrote: > > > On 11/23/18 7:02 PM, Dr. David Alan Gilbert wrote: > > > > +#include "qemu/osdep.h" > > > +#include "qemu/bitmap.h" > > > +#include "qemu/threaded-workqueue.h" > > > + > > > +#define SMP_CACHE_BYTES 64 > > > > That's architecture dependent isn't it? > > > > Yes, it's arch dependent indeed. > > I just used 64 for simplification and i think it is <= 64 on most CPU arch-es > so that can work. > > Should i introduce statically defined CACHE LINE SIZE for all arch-es? :( I think it depends why you need it; but we shouldn't have a constant that is wrong, and we shouldn't define something architecture dependent in here. > > > + /* > > > + * the bit in these two bitmaps indicates the index of the @requests > > > + * respectively. If it's the same, the corresponding request is free > > > + * and owned by the user, i.e, where the user fills a request. Otherwise, > > > + * it is valid and owned by the thread, i.e, where the thread fetches > > > + * the request and write the result. > > > + */ > > > + > > > + /* after the user fills the request, the bit is flipped. */ > > > + uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES); > > > + /* after handles the request, the thread flips the bit. */ > > > + uint64_t request_done_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES); > > > > Patchew complained about some type mismatches; I think those are because > > you're using the bitmap_* functions on these; those functions always > > operate on 'long' not on uint64_t - and on some platforms they're > > unfortunately not the same. > > I guess you were taking about this error: > ERROR: externs should be avoided in .c files > #233: FILE: util/threaded-workqueue.c:65: > + uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES); > > The complained thing is "QEMU_ALIGNED(SMP_CACHE_BYTES)" as it gone > when the aligned thing is removed... > > The issue you pointed out can be avoid by using type-casting, like: > bitmap_xor(..., (void *)&thread->request_fill_bitmap) > cannot we? I thought the error was just due to long vs uint64_t ratehr than the qemu_aligned. I don't think it's just a casting problem, since I don't think the long's are always 64bit. Dave > Thanks! -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
WARNING: multiple messages have this Message-ID (diff)
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> To: Xiao Guangrong <guangrong.xiao@gmail.com> Cc: pbonzini@redhat.com, mst@redhat.com, mtosatti@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, peterx@redhat.com, wei.w.wang@intel.com, jiang.biao2@zte.com.cn, eblake@redhat.com, quintela@redhat.com, cota@braap.org, Xiao Guangrong <xiaoguangrong@tencent.com> Subject: Re: [Qemu-devel] [PATCH v3 2/5] util: introduce threaded workqueue Date: Mon, 26 Nov 2018 10:56:22 +0000 [thread overview] Message-ID: <20181126105622.GB2547@work-vm> (raw) In-Reply-To: <e60f648f-d475-a88b-b0c3-300f18a7d36d@gmail.com> * Xiao Guangrong (guangrong.xiao@gmail.com) wrote: > > > On 11/23/18 7:02 PM, Dr. David Alan Gilbert wrote: > > > > +#include "qemu/osdep.h" > > > +#include "qemu/bitmap.h" > > > +#include "qemu/threaded-workqueue.h" > > > + > > > +#define SMP_CACHE_BYTES 64 > > > > That's architecture dependent isn't it? > > > > Yes, it's arch dependent indeed. > > I just used 64 for simplification and i think it is <= 64 on most CPU arch-es > so that can work. > > Should i introduce statically defined CACHE LINE SIZE for all arch-es? :( I think it depends why you need it; but we shouldn't have a constant that is wrong, and we shouldn't define something architecture dependent in here. > > > + /* > > > + * the bit in these two bitmaps indicates the index of the @requests > > > + * respectively. If it's the same, the corresponding request is free > > > + * and owned by the user, i.e, where the user fills a request. Otherwise, > > > + * it is valid and owned by the thread, i.e, where the thread fetches > > > + * the request and write the result. > > > + */ > > > + > > > + /* after the user fills the request, the bit is flipped. */ > > > + uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES); > > > + /* after handles the request, the thread flips the bit. */ > > > + uint64_t request_done_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES); > > > > Patchew complained about some type mismatches; I think those are because > > you're using the bitmap_* functions on these; those functions always > > operate on 'long' not on uint64_t - and on some platforms they're > > unfortunately not the same. > > I guess you were taking about this error: > ERROR: externs should be avoided in .c files > #233: FILE: util/threaded-workqueue.c:65: > + uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES); > > The complained thing is "QEMU_ALIGNED(SMP_CACHE_BYTES)" as it gone > when the aligned thing is removed... > > The issue you pointed out can be avoid by using type-casting, like: > bitmap_xor(..., (void *)&thread->request_fill_bitmap) > cannot we? I thought the error was just due to long vs uint64_t ratehr than the qemu_aligned. I don't think it's just a casting problem, since I don't think the long's are always 64bit. Dave > Thanks! -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2018-11-26 10:56 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-22 7:20 [PATCH v3 0/5] migration: improve multithreads guangrong.xiao 2018-11-22 7:20 ` [Qemu-devel] " guangrong.xiao 2018-11-22 7:20 ` [PATCH v3 1/5] bitops: introduce change_bit_atomic guangrong.xiao 2018-11-22 7:20 ` [Qemu-devel] " guangrong.xiao 2018-11-23 10:23 ` Dr. David Alan Gilbert 2018-11-23 10:23 ` [Qemu-devel] " Dr. David Alan Gilbert 2018-11-28 9:35 ` Juan Quintela 2018-11-28 9:35 ` [Qemu-devel] " Juan Quintela 2018-11-22 7:20 ` [PATCH v3 2/5] util: introduce threaded workqueue guangrong.xiao 2018-11-22 7:20 ` [Qemu-devel] " guangrong.xiao 2018-11-23 11:02 ` Dr. David Alan Gilbert 2018-11-23 11:02 ` [Qemu-devel] " Dr. David Alan Gilbert 2018-11-26 7:57 ` Xiao Guangrong 2018-11-26 7:57 ` [Qemu-devel] " Xiao Guangrong 2018-11-26 10:56 ` Dr. David Alan Gilbert [this message] 2018-11-26 10:56 ` Dr. David Alan Gilbert 2018-11-27 7:17 ` Xiao Guangrong 2018-11-27 7:17 ` [Qemu-devel] " Xiao Guangrong 2018-11-26 18:55 ` Emilio G. Cota 2018-11-26 18:55 ` [Qemu-devel] " Emilio G. Cota 2018-11-27 8:30 ` Xiao Guangrong 2018-11-27 8:30 ` [Qemu-devel] " Xiao Guangrong 2018-11-24 0:12 ` Emilio G. Cota 2018-11-24 0:12 ` [Qemu-devel] " Emilio G. Cota 2018-11-26 8:06 ` Xiao Guangrong 2018-11-26 8:06 ` [Qemu-devel] " Xiao Guangrong 2018-11-26 18:49 ` Emilio G. Cota 2018-11-26 18:49 ` [Qemu-devel] " Emilio G. Cota 2018-11-27 8:29 ` Xiao Guangrong 2018-11-27 8:29 ` [Qemu-devel] " Xiao Guangrong 2018-11-24 0:17 ` Emilio G. Cota 2018-11-24 0:17 ` [Qemu-devel] " Emilio G. Cota 2018-11-26 8:18 ` Xiao Guangrong 2018-11-26 8:18 ` [Qemu-devel] " Xiao Guangrong 2018-11-26 10:28 ` Paolo Bonzini 2018-11-26 10:28 ` [Qemu-devel] " Paolo Bonzini 2018-11-27 8:31 ` Xiao Guangrong 2018-11-27 8:31 ` [Qemu-devel] " Xiao Guangrong 2018-11-27 12:49 ` Christophe de Dinechin 2018-11-27 12:49 ` [Qemu-devel] " Christophe de Dinechin 2018-11-27 13:51 ` Paolo Bonzini 2018-11-27 13:51 ` [Qemu-devel] " Paolo Bonzini 2018-12-04 15:49 ` Christophe de Dinechin 2018-12-04 15:49 ` [Qemu-devel] " Christophe de Dinechin 2018-12-04 17:16 ` Paolo Bonzini 2018-12-04 17:16 ` [Qemu-devel] " Paolo Bonzini 2018-12-10 3:23 ` Xiao Guangrong 2018-12-10 3:23 ` [Qemu-devel] " Xiao Guangrong 2018-11-27 17:39 ` Emilio G. Cota 2018-11-27 17:39 ` [Qemu-devel] " Emilio G. Cota 2018-11-28 8:55 ` Xiao Guangrong 2018-11-28 8:55 ` [Qemu-devel] " Xiao Guangrong 2018-11-22 7:20 ` [PATCH v3 3/5] migration: use threaded workqueue for compression guangrong.xiao 2018-11-22 7:20 ` [Qemu-devel] " guangrong.xiao 2018-11-23 18:17 ` Dr. David Alan Gilbert 2018-11-23 18:17 ` [Qemu-devel] " Dr. David Alan Gilbert 2018-11-23 18:22 ` Paolo Bonzini 2018-11-23 18:22 ` [Qemu-devel] " Paolo Bonzini 2018-11-23 18:29 ` Dr. David Alan Gilbert 2018-11-23 18:29 ` [Qemu-devel] " Dr. David Alan Gilbert 2018-11-26 8:00 ` Xiao Guangrong 2018-11-26 8:00 ` [Qemu-devel] " Xiao Guangrong 2018-11-22 7:20 ` [PATCH v3 4/5] migration: use threaded workqueue for decompression guangrong.xiao 2018-11-22 7:20 ` [Qemu-devel] " guangrong.xiao 2018-11-22 7:20 ` [PATCH v3 5/5] tests: add threaded-workqueue-bench guangrong.xiao 2018-11-22 7:20 ` [Qemu-devel] " guangrong.xiao 2018-11-22 21:25 ` [PATCH v3 0/5] migration: improve multithreads no-reply 2018-11-22 21:25 ` [Qemu-devel] " no-reply 2018-11-22 21:35 ` no-reply 2018-11-22 21:35 ` [Qemu-devel] " no-reply
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181126105622.GB2547@work-vm \ --to=dgilbert@redhat.com \ --cc=cota@braap.org \ --cc=guangrong.xiao@gmail.com \ --cc=jiang.biao2@zte.com.cn \ --cc=kvm@vger.kernel.org \ --cc=mst@redhat.com \ --cc=mtosatti@redhat.com \ --cc=pbonzini@redhat.com \ --cc=peterx@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=quintela@redhat.com \ --cc=wei.w.wang@intel.com \ --cc=xiaoguangrong@tencent.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.