On Thu, 13 Oct 2005, Dave Hansen wrote:
> On Tue, 2005-10-11 at 16:12 +0100, Mel Gorman wrote:
> > + extern int get_pageblock_type(struct zone *zone,
> > + struct page *page);
>
> That indenting is pretty funky.
>
Unnecessary as well.
> > @@ -473,6 +491,15 @@ extern struct pglist_data contig_page_da
> > #if (MAX_ORDER - 1 + PAGE_SHIFT) > SECTION_SIZE_BITS
> > #error Allocator MAX_ORDER exceeds SECTION_SIZE
> > #endif
> > +#if ((SECTION_SIZE_BITS - MAX_ORDER) * BITS_PER_RCLM_TYPE) > 64
> > +#error free_area_usemap is not big enough
> > +#endif
>
> Every time I look at these patches, I see this #if, and I don't remember
> what that '64' means. Can it please get a real name?
>
Joel, suggestions?
> > +/* Usemap initialisation */
> > +#ifdef CONFIG_SPARSEMEM
> > +static inline void setup_usemap(struct pglist_data *pgdat,
> > + struct zone *zone, unsigned long zonesize) {}
> > +#endif /* CONFIG_SPARSEMEM */
> >
> > struct page;
> > struct mem_section {
> > @@ -485,6 +512,7 @@ struct mem_section {
> > * before using it wrong.
> > */
> > unsigned long section_mem_map;
> > + DECLARE_BITMAP(free_area_usemap,64);
> > };
>
> There's that '64' again! You need a space after the comma, too.
>
> > #ifdef CONFIG_SPARSEMEM_EXTREME
> > @@ -552,6 +580,17 @@ static inline struct mem_section *__pfn_
> > return __nr_to_section(pfn_to_section_nr(pfn));
> > }
> >
> > +static inline unsigned long *pfn_to_usemap(struct zone *zone, unsigned long pfn)
> > +{
> > + return &__pfn_to_section(pfn)->free_area_usemap[0];
> > +}
> > +
> > +static inline int pfn_to_bitidx(struct zone *zone, unsigned long pfn)
> > +{
> > + pfn &= (PAGES_PER_SECTION-1);
> > + return (int)((pfn >> (MAX_ORDER-1)) * BITS_PER_RCLM_TYPE);
> > +}
>
> Why does that return int? Should it be "unsigned long", maybe? Also,
> that cast is implicit in the return and shouldn't be needed.
>
It returns int because the bit functions like assign_bit() expect an int
for the bit index, not an unsigned long or anything else.
> > #define pfn_to_page(pfn) \
> > ({ \
> > unsigned long __pfn = (pfn); \
> > @@ -589,6 +628,16 @@ void sparse_init(void);
> > #else
> > #define sparse_init() do {} while (0)
> > #define sparse_index_init(_sec, _nid) do {} while (0)
> > +static inline unsigned long *pfn_to_usemap(struct zone *zone,
> > + unsigned long pfn)
> > +{
> > + return (zone->free_area_usemap);
> > +}
>
> "return" is not a function. The parenthesis can go away.
>
> > +static inline int pfn_to_bitidx(struct zone *zone, unsigned long pfn)
> > +{
> > + pfn = pfn - zone->zone_start_pfn;
> > + return (int)((pfn >> (MAX_ORDER-1)) * BITS_PER_RCLM_TYPE);
> > +}
>
> Again, why the cast?
>
Same reason as above, bit functions expect an int for the index.
> > #endif /* CONFIG_SPARSEMEM */
> >
> > #ifdef CONFIG_NODES_SPAN_OTHER_NODES
> > diff -rup -X /usr/src/patchset-0.5/bin//dontdiff linux-2.6.14-rc3-001_antidefrag_flags/mm/page_alloc.c linux-2.6.14-rc3-002_usemap/mm/page_alloc.c
> > --- linux-2.6.14-rc3-001_antidefrag_flags/mm/page_alloc.c 2005-10-04 22:58:34.000000000 +0100
> > +++ linux-2.6.14-rc3-002_usemap/mm/page_alloc.c 2005-10-11 12:07:26.000000000 +0100
> > @@ -66,6 +66,88 @@ EXPORT_SYMBOL(totalram_pages);
> > EXPORT_SYMBOL(nr_swap_pages);
> >
> > /*
> > + * RCLM_SHIFT is the number of bits that a gfp_mask has to be shifted right
> > + * to have just the __GFP_USER and __GFP_KERNRCLM bits. The static check is
> > + * made afterwards in case the GFP flags are not updated without updating
> > + * this number
> > + */
> > +#define RCLM_SHIFT 19
> > +#if (__GFP_USER >> RCLM_SHIFT) != RCLM_USER
> > +#error __GFP_USER not mapping to RCLM_USER
> > +#endif
> > +#if (__GFP_KERNRCLM >> RCLM_SHIFT) != RCLM_KERN
> > +#error __GFP_KERNRCLM not mapping to RCLM_KERN
> > +#endif
>
> Should this really be in page_alloc.c, or should it be close to the
> RCLM_* definitions?
>
I can't test it right now, but I think the reason it is here is because
RCLM_* and __GFP_* are in different headers that are not aware of each
other. This is the place a static compile-time check can be made.
> > +/*
> > + * This function maps gfpflags to their RCLM_TYPE. It makes assumptions
> > + * on the location of the GFP flags
> > + */
> > +static inline unsigned long gfpflags_to_rclmtype(unsigned long gfp_flags) {
> > + return (gfp_flags & __GFP_RCLM_BITS) >> RCLM_SHIFT;
> > +}
>
> First brace on a new line, please.
>
> > +/*
> > + * copy_bits - Copy bits between bitmaps
> > + * @dstaddr: The destination bitmap to copy to
> > + * @srcaddr: The source bitmap to copy from
> > + * @sindex_dst: The start bit index within the destination map to copy to
> > + * @sindex_src: The start bit index within the source map to copy from
> > + * @nr: The number of bits to copy
> > + */
> > +static inline void copy_bits(unsigned long *dstaddr,
> > + unsigned long *srcaddr,
> > + int sindex_dst,
> > + int sindex_src,
> > + int nr)
> > +{
> > + /*
> > + * Written like this to take advantage of arch-specific
> > + * set_bit() and clear_bit() functions
> > + */
> > + for (nr = nr-1; nr >= 0; nr--) {
> > + int bit = test_bit(sindex_src + nr, srcaddr);
> > + if (bit)
> > + set_bit(sindex_dst + nr, dstaddr);
> > + else
> > + clear_bit(sindex_dst + nr, dstaddr);
> > + }
> > +}
>
> We might want to make a note that this is slow, and doesn't have any
> atomicity guarantees, either.
The comment can be put in, but this is only called with a spinlock held.
No arguements about it being slow though.
> But, it's functional, and certainly
> describes the operation that we wanted. It's an improvement over what
> was there.
>
> BTW, this could probably be done with some fancy bitmask operations, and
> be more efficient.
>
Very likely but I was worred about corner cases if the value of
BITS_PER_RCLM_TYPE changed later to 3 or something. This time, I was going
for the safe choice.
> > +int get_pageblock_type(struct zone *zone,
> > + struct page *page)
> > +{
> > + unsigned long pfn = page_to_pfn(page);
> > + unsigned long type = 0;
> > + unsigned long *usemap;
> > + int bitidx;
> > +
> > + bitidx = pfn_to_bitidx(zone, pfn);
> > + usemap = pfn_to_usemap(zone, pfn);
> > +
> > + copy_bits(&type, usemap, 0, bitidx, BITS_PER_RCLM_TYPE);
> > +
> > + return (int)type;
> > +}
>
> Where did all these casts come from?
>
It was pointed out that type used for use with the bit functions should
all be unsigned long, not int as they were previously. However, I found if
I used unsigned long throughout the code, including for array operations,
there was a 10-12% slowdown in AIM9. These casts were the compromise.
alloctype is unsigned long when used with the functions like assign_bit()
but int every other time.
In this case, there is an implicit cast so the cast is redundent if that
is the problem you are pointing out. I can remove the explicit casts that
are dotted around the place.
> > -static struct page *__rmqueue(struct zone *zone, unsigned int order)
> > +static struct page *__rmqueue(struct zone *zone, unsigned int order,
> > + int alloctype)
> > {
> > struct free_area * area;
> > unsigned int current_order;
> > @@ -486,6 +569,15 @@ static struct page *__rmqueue(struct zon
> > rmv_page_order(page);
> > area->nr_free--;
> > zone->free_pages -= 1UL << order;
> > +
> > + /*
> > + * If splitting a large block, record what the block is being
> > + * used for in the usemap
> > + */
> > + if (current_order == MAX_ORDER-1)
> > + set_pageblock_type(zone, page,
> > + (unsigned long)alloctype);
>
> This cast makes that line wrap, and makes it quite a bit less readable.
>
> > return expand(zone, page, order, current_order, area);
> > }
> >
> > @@ -498,7 +590,8 @@ static struct page *__rmqueue(struct zon
> > * Returns the number of new pages which were placed at *list.
> > */
> > static int rmqueue_bulk(struct zone *zone, unsigned int order,
> > - unsigned long count, struct list_head *list)
> > + unsigned long count, struct list_head *list,
> > + int alloctype)
> > {
> > unsigned long flags;
> > int i;
> > @@ -507,7 +600,7 @@ static int rmqueue_bulk(struct zone *zon
> >
> > spin_lock_irqsave(&zone->lock, flags);
> > for (i = 0; i < count; ++i) {
> > - page = __rmqueue(zone, order);
> > + page = __rmqueue(zone, order, alloctype);
> > if (page == NULL)
> > break;
> > allocated++;
> > @@ -691,7 +784,8 @@ buffered_rmqueue(struct zone *zone, int
> > unsigned long flags;
> > struct page *page = NULL;
> > int cold = !!(gfp_flags & __GFP_COLD);
> > -
> > + int alloctype = (int)gfpflags_to_rclmtype(gfp_flags);
> > +
>
> Just a note: that inserts whitespace.
>
> You can avoid these kinds of mistakes by keeping incremental patches.
> Start with your last set of work, and keep all changes to _that_ work in
> separate patches on top of the old. You'll see only what you change and
> little tiny mishaps like this will tend to jump out more than in the
> hundreds of lines of the whole thing.
>
Understood.
--
Mel Gorman
Part-time Phd Student Java Applications Developer
University of Limerick IBM Dublin Software Lab
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
