Messages in this thread | | | Date | Tue, 24 Sep 2013 00:36:19 +0800 | From | Zhang Yanfei <> | Subject | Re: [PATCH v3 1/5] memblock: Introduce allocation direction to memblock. |
| |
Hello tejun,
On 09/23/2013 11:38 PM, Tejun Heo wrote: > Hello, > > Sorry about the delay. Was traveling.
hoho~ I guess you did have a good time.
> > On Fri, Sep 13, 2013 at 05:30:51PM +0800, Tang Chen wrote: >> +/* Allocation order. */ >> +#define MEMBLOCK_DIRECTION_HIGH_TO_LOW 0 >> +#define MEMBLOCK_DIRECTION_LOW_TO_HIGH 1 >> +#define MEMBLOCK_DIRECTION_DEFAULT MEMBLOCK_DIRECTION_HIGH_TO_LOW > > Can we please settle on either top_down/bottom_up or > high_to_low/low_to_high? The two seem to be used interchangeably in > the patch series. Also, it'd be more customary to use enum for things > like above, but more on the interface below.
OK. let's use top_down/bottom_up. And using enum is also ok.
> >> +static inline bool memblock_direction_bottom_up(void) >> +{ >> + return memblock.current_direction == MEMBLOCK_DIRECTION_LOW_TO_HIGH; >> +} > > Maybe just memblock_bottom_up() would be enough?
Agreed.
> > Also, why not also have memblock_set_bottom_up(bool enable) as the > 'set' interface?
hmmm, ok. So we will use memblock_set_bottom_up to replace memblock_set_current_direction below.
> >> /** >> + * memblock_set_current_direction - Set current allocation direction to allow >> + * allocating memory from higher to lower >> + * address or from lower to higher address >> + * >> + * @direction: In which order to allocate memory. Could be >> + * MEMBLOCK_DIRECTION_{HIGH_TO_LOW|LOW_TO_HIGH} >> + */ >> +void memblock_set_current_direction(int direction); > > Function comments should go with the function definition. Dunno what > happened with set_current_limit but let's please not spread it. > >> +void __init_memblock memblock_set_current_direction(int direction) >> +{ >> + if (direction != MEMBLOCK_DIRECTION_HIGH_TO_LOW && >> + direction != MEMBLOCK_DIRECTION_LOW_TO_HIGH) { >> + pr_warn("memblock: Failed to set allocation order. " >> + "Invalid order type: %d\n", direction); >> + return; >> + } >> + >> + memblock.current_direction = direction; >> +} > > If set_bottom_up() style interface is used, the above will be a lot > simpler, right? Also, it's kinda weird to have two separate patches > to introduce the flag and actually implement bottom up allocation.
Yeah, right, that'd be much simpler. And it is ok to put the two in one patch.
Thanks.
> > Thanks. >
-- Thanks. Zhang Yanfei
| |