Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1232422 > unrolled thread
| Started by | Johannes Weiner <hannes@cmpxchg.org> |
|---|---|
| First post | 2015-09-24 22:40 +0200 |
| Last post | 2015-09-25 16:00 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types Johannes Weiner <hannes@cmpxchg.org> - 2015-09-24 22:40 +0200
Re: [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types Mel Gorman <mgorman@techsingularity.net> - 2015-09-25 15:00 +0200
Re: [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types Johannes Weiner <hannes@cmpxchg.org> - 2015-09-25 16:00 +0200
| From | Johannes Weiner <hannes@cmpxchg.org> |
|---|---|
| Date | 2015-09-24 22:40 +0200 |
| Subject | Re: [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types |
| Message-ID | <qclt7-5sk-7@gated-at.bofh.it> |
On Mon, Sep 21, 2015 at 11:52:36AM +0100, Mel Gorman wrote:
> @@ -14,7 +14,7 @@ struct vm_area_struct;
> #define ___GFP_HIGHMEM 0x02u
> #define ___GFP_DMA32 0x04u
> #define ___GFP_MOVABLE 0x08u
> -#define ___GFP_WAIT 0x10u
> +#define ___GFP_RECLAIMABLE 0x10u
> #define ___GFP_HIGH 0x20u
> #define ___GFP_IO 0x40u
> #define ___GFP_FS 0x80u
> @@ -29,7 +29,7 @@ struct vm_area_struct;
> #define ___GFP_NOMEMALLOC 0x10000u
> #define ___GFP_HARDWALL 0x20000u
> #define ___GFP_THISNODE 0x40000u
> -#define ___GFP_RECLAIMABLE 0x80000u
> +#define ___GFP_WAIT 0x80000u
> #define ___GFP_NOACCOUNT 0x100000u
> #define ___GFP_NOTRACK 0x200000u
> #define ___GFP_NO_KSWAPD 0x400000u
> @@ -126,6 +126,7 @@ struct vm_area_struct;
>
> /* This mask makes up all the page movable related flags */
> #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> +#define GFP_MOVABLE_SHIFT 3
This connects the power-of-two gfp bits to the linear migrate type
enum, so shifting back and forth between them works only with up to
two items. A hypothetical ___GFP_FOOABLE would translate to 4, not
3. I'm not expecting new migratetypes to show up anytime soon, but
this implication does not make the code exactly robust and obvious.
> @@ -152,14 +153,15 @@ struct vm_area_struct;
> /* Convert GFP flags to their corresponding migrate type */
> static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> {
> - WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> + VM_WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> + BUILD_BUG_ON((1UL << GFP_MOVABLE_SHIFT) != ___GFP_MOVABLE);
> + BUILD_BUG_ON((___GFP_MOVABLE >> GFP_MOVABLE_SHIFT) != MIGRATE_MOVABLE);
>
> if (unlikely(page_group_by_mobility_disabled))
> return MIGRATE_UNMOVABLE;
>
> /* Group based on mobility */
> - return (((gfp_flags & __GFP_MOVABLE) != 0) << 1) |
> - ((gfp_flags & __GFP_RECLAIMABLE) != 0);
> + return (gfp_flags & GFP_MOVABLE_MASK) >> GFP_MOVABLE_SHIFT;
I'm not sure the simplification of this line is worth the fragile
dependency between those two tables.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2015-09-25 15:00 +0200 |
| Message-ID | <qcALw-1Ox-11@gated-at.bofh.it> |
| In reply to | #1232422 |
On Thu, Sep 24, 2015 at 04:34:45PM -0400, Johannes Weiner wrote:
> On Mon, Sep 21, 2015 at 11:52:36AM +0100, Mel Gorman wrote:
> > @@ -14,7 +14,7 @@ struct vm_area_struct;
> > #define ___GFP_HIGHMEM 0x02u
> > #define ___GFP_DMA32 0x04u
> > #define ___GFP_MOVABLE 0x08u
> > -#define ___GFP_WAIT 0x10u
> > +#define ___GFP_RECLAIMABLE 0x10u
> > #define ___GFP_HIGH 0x20u
> > #define ___GFP_IO 0x40u
> > #define ___GFP_FS 0x80u
> > @@ -29,7 +29,7 @@ struct vm_area_struct;
> > #define ___GFP_NOMEMALLOC 0x10000u
> > #define ___GFP_HARDWALL 0x20000u
> > #define ___GFP_THISNODE 0x40000u
> > -#define ___GFP_RECLAIMABLE 0x80000u
> > +#define ___GFP_WAIT 0x80000u
> > #define ___GFP_NOACCOUNT 0x100000u
> > #define ___GFP_NOTRACK 0x200000u
> > #define ___GFP_NO_KSWAPD 0x400000u
> > @@ -126,6 +126,7 @@ struct vm_area_struct;
> >
> > /* This mask makes up all the page movable related flags */
> > #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> > +#define GFP_MOVABLE_SHIFT 3
>
> This connects the power-of-two gfp bits to the linear migrate type
> enum, so shifting back and forth between them works only with up to
> two items. A hypothetical ___GFP_FOOABLE would translate to 4, not
> 3. I'm not expecting new migratetypes to show up anytime soon, but
> this implication does not make the code exactly robust and obvious.
>
In the event __GFP_FOOABLE is added then it would need to be reverted.
Adding new migrate types has other consequences as it increases the number
of free lists in struct zone and depending on the type then new per-cpu
lists and the the fallbacks have to be updated. It's fairly obvious if a
new migratetype is added that cares.
> > @@ -152,14 +153,15 @@ struct vm_area_struct;
> > /* Convert GFP flags to their corresponding migrate type */
> > static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> > {
> > - WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> > + VM_WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> > + BUILD_BUG_ON((1UL << GFP_MOVABLE_SHIFT) != ___GFP_MOVABLE);
> > + BUILD_BUG_ON((___GFP_MOVABLE >> GFP_MOVABLE_SHIFT) != MIGRATE_MOVABLE);
> >
> > if (unlikely(page_group_by_mobility_disabled))
> > return MIGRATE_UNMOVABLE;
> >
> > /* Group based on mobility */
> > - return (((gfp_flags & __GFP_MOVABLE) != 0) << 1) |
> > - ((gfp_flags & __GFP_RECLAIMABLE) != 0);
> > + return (gfp_flags & GFP_MOVABLE_MASK) >> GFP_MOVABLE_SHIFT;
>
> I'm not sure the simplification of this line is worth the fragile
> dependency between those two tables.
The BUILD_BUG_ON is there to blow up immediately if the dependency is
broken. Sure, it's complexity but it's well contained in a single
place. Do you want to insist the patch is dropped in case someone
decides to add a new migrate type that has per-cpu lists?
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Johannes Weiner <hannes@cmpxchg.org> |
|---|---|
| Date | 2015-09-25 16:00 +0200 |
| Message-ID | <qcBHA-3cl-17@gated-at.bofh.it> |
| In reply to | #1232778 |
On Fri, Sep 25, 2015 at 01:50:28PM +0100, Mel Gorman wrote:
> On Thu, Sep 24, 2015 at 04:34:45PM -0400, Johannes Weiner wrote:
> > On Mon, Sep 21, 2015 at 11:52:36AM +0100, Mel Gorman wrote:
> > > @@ -14,7 +14,7 @@ struct vm_area_struct;
> > > #define ___GFP_HIGHMEM 0x02u
> > > #define ___GFP_DMA32 0x04u
> > > #define ___GFP_MOVABLE 0x08u
> > > -#define ___GFP_WAIT 0x10u
> > > +#define ___GFP_RECLAIMABLE 0x10u
> > > #define ___GFP_HIGH 0x20u
> > > #define ___GFP_IO 0x40u
> > > #define ___GFP_FS 0x80u
> > > @@ -29,7 +29,7 @@ struct vm_area_struct;
> > > #define ___GFP_NOMEMALLOC 0x10000u
> > > #define ___GFP_HARDWALL 0x20000u
> > > #define ___GFP_THISNODE 0x40000u
> > > -#define ___GFP_RECLAIMABLE 0x80000u
> > > +#define ___GFP_WAIT 0x80000u
> > > #define ___GFP_NOACCOUNT 0x100000u
> > > #define ___GFP_NOTRACK 0x200000u
> > > #define ___GFP_NO_KSWAPD 0x400000u
> > > @@ -126,6 +126,7 @@ struct vm_area_struct;
> > >
> > > /* This mask makes up all the page movable related flags */
> > > #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> > > +#define GFP_MOVABLE_SHIFT 3
> >
> > This connects the power-of-two gfp bits to the linear migrate type
> > enum, so shifting back and forth between them works only with up to
> > two items. A hypothetical ___GFP_FOOABLE would translate to 4, not
> > 3. I'm not expecting new migratetypes to show up anytime soon, but
> > this implication does not make the code exactly robust and obvious.
> >
>
> In the event __GFP_FOOABLE is added then it would need to be reverted.
> Adding new migrate types has other consequences as it increases the number
> of free lists in struct zone and depending on the type then new per-cpu
> lists and the the fallbacks have to be updated. It's fairly obvious if a
> new migratetype is added that cares.
As I said, I'm not really worried about somebody screwing up when
actually adding a new migratetype, but rather people having a harder
time reviewing and verifying the code later on and going through the
same "how can it translate a power of two to linear with a shift op?
oh, it relies on the sequences coinciding for the first two elements"
like I did. The instance works fine, but it's a questionable pattern.
> > > @@ -152,14 +153,15 @@ struct vm_area_struct;
> > > /* Convert GFP flags to their corresponding migrate type */
> > > static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> > > {
> > > - WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> > > + VM_WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> > > + BUILD_BUG_ON((1UL << GFP_MOVABLE_SHIFT) != ___GFP_MOVABLE);
> > > + BUILD_BUG_ON((___GFP_MOVABLE >> GFP_MOVABLE_SHIFT) != MIGRATE_MOVABLE);
> > >
> > > if (unlikely(page_group_by_mobility_disabled))
> > > return MIGRATE_UNMOVABLE;
> > >
> > > /* Group based on mobility */
> > > - return (((gfp_flags & __GFP_MOVABLE) != 0) << 1) |
> > > - ((gfp_flags & __GFP_RECLAIMABLE) != 0);
> > > + return (gfp_flags & GFP_MOVABLE_MASK) >> GFP_MOVABLE_SHIFT;
> >
> > I'm not sure the simplification of this line is worth the fragile
> > dependency between those two tables.
>
> The BUILD_BUG_ON is there to blow up immediately if the dependency is
> broken. Sure, it's complexity but it's well contained in a single
> place. Do you want to insist the patch is dropped in case someone
> decides to add a new migrate type that has per-cpu lists?
I think the previous code is easier to comprehend and the optimization
looks miniscule. It doesn't seem worth it to me to add complications
and subtleties to a complex codebase for microoptimizations whose
effects are likely in the noise.
OTOH several others have acked this patch. If nobody else shares my
concern then I'm not insisting that the patch be dropped.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web