2018-09-12 05:57:40

by zhong jiang

[permalink] [raw]
Subject: [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

DIV_ROUND_UP has implemented the code-opened function. Therefore, just
replace the implementation with DIV_ROUND_UP.

Signed-off-by: zhong jiang <[email protected]>
---
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



2018-09-12 08:14:54

by Jan Beulich

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

>>> On 12.09.18 at 07:45, <[email protected]> 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



2018-09-12 09:15:24

by Roger Pau Monne

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

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, <[email protected]> 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.

2018-09-12 09:17:31

by Roger Pau Monne

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

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, <[email protected]> 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
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

2018-09-12 09:49:07

by Julien Grall

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

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, <[email protected]> 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

2018-09-12 10:29:45

by Roger Pau Monne

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

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, <[email protected]> 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.

2018-09-24 13:28:54

by Julien Grall

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

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, <[email protected]> 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

2018-10-02 07:44:21

by Roger Pau Monne

[permalink] [raw]
Subject: Re: [Xen-devel] [PATCH] drivers/block/xen-blkback/common.h: use DIV_ROUND_UP instead of reimplementing its function

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, <[email protected]> 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.