Re: [PATCH V7 1/3] genalloc:support memory-allocation with bytes-alignment to genalloc

[Scott Wood]

On Tue, 2015-09-01 at 22:05 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 02, 2015 10:33 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx;
> > lauraa@xxxxxxxxxxxxxx; Xie Xiaobo-R63061; benh@xxxxxxxxxxxxxxxxxxx; Li
> > Yang-Leo-R58472; paulus@xxxxxxxxx
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Tue, 2015-09-01 at 21:29 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 02, 2015 10:18 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx;
> > > > lauraa@xxxxxxxxxxxxxx; Xie Xiaobo-R63061; benh@xxxxxxxxxxxxxxxxxxx;
> > > > Li Yang-Leo-R58472; paulus@xxxxxxxxx
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > > > To: Zhao Qiang-B45475
> > > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linuxppc-dev@xxxxxxxxxxxxxxxx;
> > > > > > lauraa@xxxxxxxxxxxxxx; Xie Xiaobo-R63061;
> > > > > > benh@xxxxxxxxxxxxxxxxxxx; Li Yang-Leo-R58472; paulus@xxxxxxxxx
> > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation
> > > > > > with bytes-alignment to genalloc
> > > > > > 
> > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > > > Bytes alignment is required to manage some special RAM, so add
> > > > > > > gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > > > gen_pool_alloc_data to pass data to
> > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)
> > > > > > > 
> > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@xxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > Changes for v6:
> > > > > > >       - patches set v6 include a new patch because of using
> > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > > > >       - patch, adding bytes alignment for allocation for use.
> > > > > > > Changes for v7:
> > > > > > >       - cpm muram also need to use genalloc to manage, it has
> > > > > > >         a function to reserve a specific region of muram,
> > > > > > >         add offset to genpool_data for start addr to be
> > allocated.
> > > > > > 
> > > > > > This seems to be describing more than just the changes in this
> > patch.
> > > > > > What does also handling cpm have to do with this patch?  Are you
> > > > > > adding support for reserving a specific region in this patch?  I
> > > > > > don't see it, and in any case it should go in a different patch.
> > > > > 
> > > > > Yes, I added. The code below can support the function.
> > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>
> > order;
> > > > >       return bitmap_find_next_zero_area(map, size, start +
> > > > > offset_bit,
> > > > nr,
> > > > >                         align_mask);
> > > > > 
> > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate
> > > > > muram from a Specific offset. So I add the code and add offset to
> > struct data.
> > > > 
> > > > I thought the offset was related to the previous discussion of
> > > > checking for allocation failure.  Are you using it to implement
> > > > alloc_fixed()?  If so, please don't.  Besides the awkward
> > > > implementation (what does it logically have to do with
> > > > gen_pool_first_fit_align?), it does not appear to be correct -
> > > > - what happens with multiple chunks?  What happens if part of the
> > > > region the caller is trying to reserve is already taken?  Implement
> > > > a proper function to reserve a fixed genalloc region.
> > > 
> > > This offset is totally different with the workaround OFFSET!
> > 
> > There's a reason why we write changelogs that describe what the patch is
> > doing, and avoid combining logically distinct changes in the same patch.
> > 
> > > This offset is the offset of the muram.
> > 
> > The offset of the muram relative to what?  Or do you mean the offset into
> > muram?
> 
> Yes, the offset into muram.
> 
> > 
> > > CPM need to allocate block from a specific offset due to hardware
> > > restriction.
> > > So I must handle this offset in genalloc.
> > 
> > Again, if you need to be able to mark a specific range reserved, add a
> > function that does that properly.  Don't try to hack it in the way you
> > did.
> 
> Add a function? Do you mean add a new alloc function or new algo?
> If you mean new algo, CPM use both align algo and new algo, set 
> Different algos in different muram_alloc func? 

I was thinking that it was a sufficiently different operation that it 
warranted its own independent function, but I suppose you could do it as an 
algorithm that only accepts the requested range and returns failure for all 
other chunks (as well as if the range is unavailable).  It would not be 
related at all to the aligned-alloc algorithm.

-Scott


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/