From: "Wang, Wei W" <wei.w.wang@intel.com>
To: Matthew Wilcox <willy@infradead.org>,
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: "virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
"mst@redhat.com" <mst@redhat.com>,
"mhocko@kernel.org" <mhocko@kernel.org>,
"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
"mawilcox@microsoft.com" <mawilcox@microsoft.com>,
"david@redhat.com" <david@redhat.com>,
"cornelia.huck@de.ibm.com" <cornelia.huck@de.ibm.com>,
"mgorman@techsingularity.net" <mgorman@techsingularity.net>,
"aarcange@redhat.com" <aarcange@redhat.com>,
"amit.shah@redhat.com" <amit.shah@redhat.com>,
"pbonzini@redhat.com" <pbonzini@redhat.com>,
"liliang.opensource@gmail.com" <liliang.opensource@gmail.com>,
"yang.zhang.wz@gmail.com" <yang.zhang.wz@gmail.com>,
"quan.xu@aliyun.com" <quan.xu@aliyun.com>,
"nilal@redhat.com" <nilal@redhat.com>,
"riel@redhat.com" <riel@redhat.com>
Subject: RE: [PATCH v19 3/7] xbitmap: add more operations
Date: Sun, 17 Dec 2017 13:47:21 +0000 [thread overview]
Message-ID: <286AC319A985734F985F78AFA26841F739387C1D@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <20171215192203.GC27160@bombadil.infradead.org>
On Saturday, December 16, 2017 3:22 AM, Matthew Wilcox wrote:
> On Fri, Dec 15, 2017 at 10:49:15AM -0800, Matthew Wilcox wrote:
> > Here's the API I'm looking at right now. The user need take no lock;
> > the locking (spinlock) is handled internally to the implementation.
Another place I saw your comment " The xb_ API requires you to handle your own locking" which seems conflict with the above "the user need take no lock".
Doesn't the caller need a lock to avoid concurrent accesses to the ida bitmap?
> I looked at the API some more and found some flaws:
> - how does xbit_alloc communicate back which bit it allocated?
> - What if xbit_find_set() is called on a completely empty array with
> a range of 0, ULONG_MAX -- there's no invalid number to return.
We'll change it to "bool xb_find_set(.., unsigned long *result)", returning false indicates no "1" bit is found.
> - xbit_clear() can't return an error. Neither can xbit_zero().
I found the current xbit_clear implementation only returns 0, and there isn't an error to be returned from this function. In this case, is it better to make the function "void"?
> - Need to add __must_check to various return values to discourage sloppy
> programming
>
> So I modify the proposed API we compete with thusly:
>
> bool xbit_test(struct xbitmap *, unsigned long bit); int __must_check
> xbit_set(struct xbitmap *, unsigned long bit, gfp_t); void xbit_clear(struct
> xbitmap *, unsigned long bit); int __must_check xbit_alloc(struct xbitmap *,
> unsigned long *bit, gfp_t);
>
> int __must_check xbit_fill(struct xbitmap *, unsigned long start,
> unsigned long nbits, gfp_t); void xbit_zero(struct xbitmap *,
> unsigned long start, unsigned long nbits); int __must_check
> xbit_alloc_range(struct xbitmap *, unsigned long *bit,
> unsigned long nbits, gfp_t);
>
> bool xbit_find_clear(struct xbitmap *, unsigned long *start, unsigned long
> max); bool xbit_find_set(struct xbitmap *, unsigned long *start, unsigned
> long max);
>
> (I'm a little sceptical about the API accepting 'max' for the find functions and
> 'nbits' in the fill/zero/alloc_range functions, but I think that matches how
> people want to use it, and it matches how bitmap.h works)
Are you suggesting to rename the current xb_ APIs to the above xbit_ names (with parameter changes)?
Why would we need xbit_alloc, which looks like ida_get_new, I think set/clear should be adequate to the current usages.
Best,
Wei
next prev parent reply other threads:[~2017-12-17 13:47 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-12 11:55 [PATCH v19 0/7] Virtio-balloon Enhancement Wei Wang
2017-12-12 11:55 ` [PATCH v19 1/7] xbitmap: Introduce xbitmap Wei Wang
2017-12-12 12:53 ` Philippe Ombredanne
2017-12-15 11:05 ` kbuild test robot
2017-12-15 13:24 ` Matthew Wilcox
2017-12-16 10:10 ` Wei Wang
2017-12-12 11:55 ` [PATCH v19 2/7] xbitmap: potential improvement Wei Wang
2017-12-15 3:07 ` kbuild test robot
2017-12-12 11:55 ` [PATCH v19 3/7] xbitmap: add more operations Wei Wang
2017-12-12 13:20 ` Tetsuo Handa
2017-12-13 12:26 ` Wei Wang
2017-12-13 14:16 ` Tetsuo Handa
2017-12-14 3:47 ` Wei Wang
2017-12-14 11:47 ` [virtio-dev] " Wei Wang
2017-12-14 16:29 ` Tetsuo Handa
2017-12-14 18:12 ` Matthew Wilcox
2017-12-15 16:21 ` Tetsuo Handa
2017-12-15 18:26 ` Michael S. Tsirkin
2017-12-16 4:31 ` Tetsuo Handa
2017-12-16 5:05 ` Matthew Wilcox
2017-12-16 5:57 ` Tetsuo Handa
2017-12-15 18:49 ` Matthew Wilcox
2017-12-15 19:22 ` Matthew Wilcox
2017-12-17 13:47 ` Wang, Wei W [this message]
2017-12-17 22:18 ` Matthew Wilcox
2017-12-18 2:33 ` Wei Wang
2017-12-18 2:59 ` Matthew Wilcox
2017-12-16 10:14 ` Wei Wang
2017-12-14 12:37 ` Matthew Wilcox
2017-12-15 18:42 ` Matthew Wilcox
2017-12-16 10:12 ` Wei Wang
2017-12-16 11:28 ` Tetsuo Handa
2017-12-17 5:24 ` Wei Wang
2017-12-17 10:21 ` Tetsuo Handa
2017-12-17 11:50 ` Wang, Wei W
2017-12-17 15:16 ` Tetsuo Handa
2017-12-18 8:05 ` Wei Wang
2017-12-12 11:55 ` [PATCH v19 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG Wei Wang
2017-12-12 11:55 ` [PATCH v19 5/7] mm: support reporting free page blocks Wei Wang
2017-12-12 11:55 ` [PATCH v19 6/7] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ Wei Wang
2017-12-12 11:55 ` [PATCH v19 7/7] virtio-balloon: don't report free pages when page poisoning is enabled Wei Wang
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=286AC319A985734F985F78AFA26841F739387C1D@shsmsx102.ccr.corp.intel.com \
--to=wei.w.wang@intel.com \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=amit.shah@redhat.com \
--cc=cornelia.huck@de.ibm.com \
--cc=david@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=liliang.opensource@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mawilcox@microsoft.com \
--cc=mgorman@techsingularity.net \
--cc=mhocko@kernel.org \
--cc=mst@redhat.com \
--cc=nilal@redhat.com \
--cc=pbonzini@redhat.com \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
--cc=qemu-devel@nongnu.org \
--cc=quan.xu@aliyun.com \
--cc=riel@redhat.com \
--cc=virtio-dev@lists.oasis-open.org \
--cc=virtualization@lists.linux-foundation.org \
--cc=willy@infradead.org \
--cc=yang.zhang.wz@gmail.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).