* [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function @ 2018-09-12 5:45 zhong jiang 2018-09-12 8:14 ` [Xen-devel] " Jan Beulich 0 siblings, 1 reply; 8+ messages in thread From: zhong jiang @ 2018-09-12 5:45 UTC (permalink / raw) To: axboe; +Cc: roger.pau, konrad.wilk, xen-devel, linux-block, linux-kernel DIV_ROUND_UP has implemented the code-opened function. Therefore, just replace the implementation with DIV_ROUND_UP. Signed-off-by: zhong jiang <zhongjiang@huawei.com> --- drivers/block/xen-blkback/common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h index 1d3002d..874613d 100644 --- a/drivers/block/xen-blkback/common.h +++ b/drivers/block/xen-blkback/common.h @@ -65,7 +65,7 @@ (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) #define MAX_INDIRECT_PAGES \ - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) /* Not a real protocol. Used to generate ring structs which contain -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-12 5:45 [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function zhong jiang @ 2018-09-12 8:14 ` Jan Beulich 2018-09-12 9:13 ` Roger Pau Monné 0 siblings, 1 reply; 8+ messages in thread From: Jan Beulich @ 2018-09-12 8:14 UTC (permalink / raw) To: Roger Pau Monne, Konrad Rzeszutek Wilk Cc: zhong jiang, Jens Axboe, xen-devel, linux-block, linux-kernel >>> On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: > --- a/drivers/block/xen-blkback/common.h > +++ b/drivers/block/xen-blkback/common.h > @@ -65,7 +65,7 @@ > (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) > > #define MAX_INDIRECT_PAGES \ > - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) > + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) > #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) My first reaction was to suggest #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) but that wouldn't match what's there currently (note the two different divisors). I can't really decide whether that's just unfortunate naming of the two macros, or an actual bug. Jan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-12 8:14 ` [Xen-devel] " Jan Beulich @ 2018-09-12 9:13 ` Roger Pau Monné 2018-09-12 9:16 ` Roger Pau Monné 0 siblings, 1 reply; 8+ messages in thread From: Roger Pau Monné @ 2018-09-12 9:13 UTC (permalink / raw) To: Jan Beulich Cc: Konrad Rzeszutek Wilk, Jens Axboe, xen-devel, zhong jiang, linux-block, linux-kernel Adding Julien how did the work to support XEN_PAGE_SIZE != PAGE_SIZE. On Wed, Sep 12, 2018 at 02:14:26AM -0600, Jan Beulich wrote: > >>> On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: > > --- a/drivers/block/xen-blkback/common.h > > +++ b/drivers/block/xen-blkback/common.h > > @@ -65,7 +65,7 @@ > > (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) > > > > #define MAX_INDIRECT_PAGES \ > > - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) > > + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) > > #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) > > My first reaction was to suggest > > #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) > > but that wouldn't match what's there currently (note the two different > divisors). I can't really decide whether that's just unfortunate naming > of the two macros, or an actual bug. I think there's indeed a bug here. AFAICT, MAX_INDIRECT_PAGES should use XEN_PAGES_PER_INDIRECT_FRAME and then it could be changed as Jan suggested. Current MAX_INDIRECT_PAGES is misnamed and should instead be MAX_INDIRECT_SEGS (which on x86 is exactly the same because PAGE_SIZE == XEN_PAGE_SIZE). Roger. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-12 9:13 ` Roger Pau Monné @ 2018-09-12 9:16 ` Roger Pau Monné 2018-09-12 9:48 ` Julien Grall 0 siblings, 1 reply; 8+ messages in thread From: Roger Pau Monné @ 2018-09-12 9:16 UTC (permalink / raw) To: Julien Grall Cc: Jan Beulich, Jens Axboe, Konrad Rzeszutek Wilk, linux-kernel, linux-block, xen-devel, zhong jiang Sorry, I've failed to add Julien in my previous reply. On Wed, Sep 12, 2018 at 11:13:50AM +0200, Roger Pau Monné wrote: > Adding Julien how did the work to support XEN_PAGE_SIZE != PAGE_SIZE. > > On Wed, Sep 12, 2018 at 02:14:26AM -0600, Jan Beulich wrote: > > >>> On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: > > > --- a/drivers/block/xen-blkback/common.h > > > +++ b/drivers/block/xen-blkback/common.h > > > @@ -65,7 +65,7 @@ > > > (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) > > > > > > #define MAX_INDIRECT_PAGES \ > > > - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) > > > + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) > > > #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) > > > > My first reaction was to suggest > > > > #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) > > > > but that wouldn't match what's there currently (note the two different > > divisors). I can't really decide whether that's just unfortunate naming > > of the two macros, or an actual bug. > > I think there's indeed a bug here. > > AFAICT, MAX_INDIRECT_PAGES should use XEN_PAGES_PER_INDIRECT_FRAME and > then it could be changed as Jan suggested. > > Current MAX_INDIRECT_PAGES is misnamed and should instead be > MAX_INDIRECT_SEGS (which on x86 is exactly the same because PAGE_SIZE > == XEN_PAGE_SIZE). > > Roger. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-12 9:16 ` Roger Pau Monné @ 2018-09-12 9:48 ` Julien Grall 2018-09-12 10:29 ` Roger Pau Monné 0 siblings, 1 reply; 8+ messages in thread From: Julien Grall @ 2018-09-12 9:48 UTC (permalink / raw) To: Roger Pau Monné Cc: Jan Beulich, Jens Axboe, Konrad Rzeszutek Wilk, linux-kernel, linux-block, xen-devel, zhong jiang Hi, On 09/12/2018 10:16 AM, Roger Pau Monné wrote: > On Wed, Sep 12, 2018 at 11:13:50AM +0200, Roger Pau Monné wrote: >> Adding Julien how did the work to support XEN_PAGE_SIZE != PAGE_SIZE. >> >> On Wed, Sep 12, 2018 at 02:14:26AM -0600, Jan Beulich wrote: >>>>>> On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: >>>> --- a/drivers/block/xen-blkback/common.h >>>> +++ b/drivers/block/xen-blkback/common.h >>>> @@ -65,7 +65,7 @@ >>>> (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) >>>> >>>> #define MAX_INDIRECT_PAGES \ >>>> - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) >>>> + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) >>>> #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) >>> >>> My first reaction was to suggest >>> >>> #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) >>> >>> but that wouldn't match what's there currently (note the two different >>> divisors). I can't really decide whether that's just unfortunate naming >>> of the two macros, or an actual bug. >> >> I think there's indeed a bug here. >> >> AFAICT, MAX_INDIRECT_PAGES should use XEN_PAGES_PER_INDIRECT_FRAME and >> then it could be changed as Jan suggested. The problem is SEGS_PER_INDIRECT_FRAME has been miscalculated. So I think it would be fine to use XEN_PAGES_PER_INDIRECT_FRAME in MAX_INDIRECT_PAGES. However the naming for XEN_PAGES_PER_INDIRECT_FRAME is misnamed. We return number of a for segments per indirect frame. So I would rename to SEGS_PER_INDIRECT_FRAME. >> >> Current MAX_INDIRECT_PAGES is misnamed and should instead be >> MAX_INDIRECT_SEGS (which on x86 is exactly the same because PAGE_SIZE >> == XEN_PAGE_SIZE). Looking at the usage: j = min(MAX_INDIRECT_PAGES, INDIRECT_PAGES(nr_segments)) Where j is used as the number of grant ref. So I don't think the variable is misnamed here. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-12 9:48 ` Julien Grall @ 2018-09-12 10:29 ` Roger Pau Monné 2018-09-24 13:28 ` Julien Grall 0 siblings, 1 reply; 8+ messages in thread From: Roger Pau Monné @ 2018-09-12 10:29 UTC (permalink / raw) To: Julien Grall Cc: Jan Beulich, Jens Axboe, Konrad Rzeszutek Wilk, linux-kernel, linux-block, xen-devel, zhong jiang On Wed, Sep 12, 2018 at 10:48:42AM +0100, Julien Grall wrote: > Hi, > > On 09/12/2018 10:16 AM, Roger Pau Monné wrote: > > On Wed, Sep 12, 2018 at 11:13:50AM +0200, Roger Pau Monné wrote: > > > Adding Julien how did the work to support XEN_PAGE_SIZE != PAGE_SIZE. > > > > > > On Wed, Sep 12, 2018 at 02:14:26AM -0600, Jan Beulich wrote: > > > > > > > On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: > > > > > --- a/drivers/block/xen-blkback/common.h > > > > > +++ b/drivers/block/xen-blkback/common.h > > > > > @@ -65,7 +65,7 @@ > > > > > (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) > > > > > #define MAX_INDIRECT_PAGES \ > > > > > - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) > > > > > + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) > > > > > #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) > > > > > > > > My first reaction was to suggest > > > > > > > > #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) > > > > > > > > but that wouldn't match what's there currently (note the two different > > > > divisors). I can't really decide whether that's just unfortunate naming > > > > of the two macros, or an actual bug. > > > > > > I think there's indeed a bug here. > > > > > > AFAICT, MAX_INDIRECT_PAGES should use XEN_PAGES_PER_INDIRECT_FRAME and > > > then it could be changed as Jan suggested. > > The problem is SEGS_PER_INDIRECT_FRAME has been miscalculated. So I think it > would be fine to use XEN_PAGES_PER_INDIRECT_FRAME in MAX_INDIRECT_PAGES. > > However the naming for XEN_PAGES_PER_INDIRECT_FRAME is misnamed. We return > number of a for segments per indirect frame. So I would rename to > SEGS_PER_INDIRECT_FRAME. I don't think I agree with this last part, SEGS_PER_INDIRECT_FRAME would have to take into account XEN_PAGES_PER_SEGMENT, and XEN_PAGES_PER_INDIRECT_FRAME doesn't. XEN_PAGES_PER_INDIRECT_FRAME currently returns the number of grant references per indirect page, but as I understand it a segment can use more than one grant reference, if for example the guest page size is 64KB. > > > > > > > Current MAX_INDIRECT_PAGES is misnamed and should instead be > > > MAX_INDIRECT_SEGS (which on x86 is exactly the same because PAGE_SIZE > > > == XEN_PAGE_SIZE). > > Looking at the usage: > > j = min(MAX_INDIRECT_PAGES, INDIRECT_PAGES(nr_segments)) > > Where j is used as the number of grant ref. So I don't think the variable is > misnamed here. Right, I agree that MAX_INDIRECT_PAGE/INDIRECT_PAGES is correct. Thanks, Roger. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-12 10:29 ` Roger Pau Monné @ 2018-09-24 13:28 ` Julien Grall 2018-10-02 7:43 ` Roger Pau Monné 0 siblings, 1 reply; 8+ messages in thread From: Julien Grall @ 2018-09-24 13:28 UTC (permalink / raw) To: Roger Pau Monné Cc: Jan Beulich, Jens Axboe, Konrad Rzeszutek Wilk, linux-kernel, linux-block, xen-devel, zhong jiang Hi Roger, On 09/12/2018 11:29 AM, Roger Pau Monné wrote: > On Wed, Sep 12, 2018 at 10:48:42AM +0100, Julien Grall wrote: >> Hi, >> >> On 09/12/2018 10:16 AM, Roger Pau Monné wrote: >>> On Wed, Sep 12, 2018 at 11:13:50AM +0200, Roger Pau Monné wrote: >>>> Adding Julien how did the work to support XEN_PAGE_SIZE != PAGE_SIZE. >>>> >>>> On Wed, Sep 12, 2018 at 02:14:26AM -0600, Jan Beulich wrote: >>>>>>>> On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: >>>>>> --- a/drivers/block/xen-blkback/common.h >>>>>> +++ b/drivers/block/xen-blkback/common.h >>>>>> @@ -65,7 +65,7 @@ >>>>>> (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) >>>>>> #define MAX_INDIRECT_PAGES \ >>>>>> - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) >>>>>> + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) >>>>>> #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) >>>>> >>>>> My first reaction was to suggest >>>>> >>>>> #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) >>>>> >>>>> but that wouldn't match what's there currently (note the two different >>>>> divisors). I can't really decide whether that's just unfortunate naming >>>>> of the two macros, or an actual bug. >>>> >>>> I think there's indeed a bug here. >>>> >>>> AFAICT, MAX_INDIRECT_PAGES should use XEN_PAGES_PER_INDIRECT_FRAME and >>>> then it could be changed as Jan suggested. >> >> The problem is SEGS_PER_INDIRECT_FRAME has been miscalculated. So I think it >> would be fine to use XEN_PAGES_PER_INDIRECT_FRAME in MAX_INDIRECT_PAGES. >> >> However the naming for XEN_PAGES_PER_INDIRECT_FRAME is misnamed. We return >> number of a for segments per indirect frame. So I would rename to >> SEGS_PER_INDIRECT_FRAME. > > I don't think I agree with this last part, SEGS_PER_INDIRECT_FRAME > would have to take into account XEN_PAGES_PER_SEGMENT, and > XEN_PAGES_PER_INDIRECT_FRAME doesn't. > > XEN_PAGES_PER_INDIRECT_FRAME currently returns the number of grant > references per indirect page, but as I understand it a segment can use > more than one grant reference, if for example the guest page size is > 64KB. I am a bit confused. By segment, do you refer to the backend or frontend segment? In any case, it would be possible to remove SEGS_PER_INDIRECT_FRAME if we rework MAX_INDIRECT_PAGES(...). This should improve the readability as well. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function 2018-09-24 13:28 ` Julien Grall @ 2018-10-02 7:43 ` Roger Pau Monné 0 siblings, 0 replies; 8+ messages in thread From: Roger Pau Monné @ 2018-10-02 7:43 UTC (permalink / raw) To: Julien Grall Cc: Jan Beulich, Jens Axboe, Konrad Rzeszutek Wilk, linux-kernel, linux-block, xen-devel, zhong jiang On Mon, Sep 24, 2018 at 02:28:26PM +0100, Julien Grall wrote: > Hi Roger, > > On 09/12/2018 11:29 AM, Roger Pau Monné wrote: > > On Wed, Sep 12, 2018 at 10:48:42AM +0100, Julien Grall wrote: > > > Hi, > > > > > > On 09/12/2018 10:16 AM, Roger Pau Monné wrote: > > > > On Wed, Sep 12, 2018 at 11:13:50AM +0200, Roger Pau Monné wrote: > > > > > Adding Julien how did the work to support XEN_PAGE_SIZE != PAGE_SIZE. > > > > > > > > > > On Wed, Sep 12, 2018 at 02:14:26AM -0600, Jan Beulich wrote: > > > > > > > > > On 12.09.18 at 07:45, <zhongjiang@huawei.com> wrote: > > > > > > > --- a/drivers/block/xen-blkback/common.h > > > > > > > +++ b/drivers/block/xen-blkback/common.h > > > > > > > @@ -65,7 +65,7 @@ > > > > > > > (XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT) > > > > > > > #define MAX_INDIRECT_PAGES \ > > > > > > > - ((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1)/SEGS_PER_INDIRECT_FRAME) > > > > > > > + DIV_ROUND_UP(MAX_INDIRECT_SEGMENTS, SEGS_PER_INDIRECT_FRAME) > > > > > > > #define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME) > > > > > > > > > > > > My first reaction was to suggest > > > > > > > > > > > > #define MAX_INDIRECT_PAGES INDIRECT_PAGES(MAX_INDIRECT_SEGMENTS) > > > > > > > > > > > > but that wouldn't match what's there currently (note the two different > > > > > > divisors). I can't really decide whether that's just unfortunate naming > > > > > > of the two macros, or an actual bug. > > > > > > > > > > I think there's indeed a bug here. > > > > > > > > > > AFAICT, MAX_INDIRECT_PAGES should use XEN_PAGES_PER_INDIRECT_FRAME and > > > > > then it could be changed as Jan suggested. > > > > > > The problem is SEGS_PER_INDIRECT_FRAME has been miscalculated. So I think it > > > would be fine to use XEN_PAGES_PER_INDIRECT_FRAME in MAX_INDIRECT_PAGES. > > > > > > However the naming for XEN_PAGES_PER_INDIRECT_FRAME is misnamed. We return > > > number of a for segments per indirect frame. So I would rename to > > > SEGS_PER_INDIRECT_FRAME. > > > > I don't think I agree with this last part, SEGS_PER_INDIRECT_FRAME > > would have to take into account XEN_PAGES_PER_SEGMENT, and > > XEN_PAGES_PER_INDIRECT_FRAME doesn't. > > > > XEN_PAGES_PER_INDIRECT_FRAME currently returns the number of grant > > references per indirect page, but as I understand it a segment can use > > more than one grant reference, if for example the guest page size is > > 64KB. > > I am a bit confused. By segment, do you refer to the backend or frontend > segment? Backend segment. I guess it's quite messy to have both frontend segment size and backend segment size which can be different. > In any case, it would be possible to remove SEGS_PER_INDIRECT_FRAME if we > rework MAX_INDIRECT_PAGES(...). This should improve the readability as well. Yes, I think this should improve the code. Roger. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-02 7:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-12 5:45 [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function zhong jiang 2018-09-12 8:14 ` [Xen-devel] " Jan Beulich 2018-09-12 9:13 ` Roger Pau Monné 2018-09-12 9:16 ` Roger Pau Monné 2018-09-12 9:48 ` Julien Grall 2018-09-12 10:29 ` Roger Pau Monné 2018-09-24 13:28 ` Julien Grall 2018-10-02 7:43 ` Roger Pau Monné
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).