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

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

[zhong jiang]

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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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.

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

[Roger Pau Monné]

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

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

[Julien Grall]

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

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

[Roger Pau Monné]

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.

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

[Julien Grall]

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

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

[Roger Pau Monné]

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.