v3.15-rc1 slab allocator broken on m68knommu (coldfire)

v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Steven King]

git bisect suggests it starts somewhere around commit 
f315e3fa1cf5b3317fc948708645fff889ce1e63 slab: restrict the number of objects 
in a slab

but its kinda hard to tell as there is some compile breakage in there as well.

slub and slob seem to still work okay for m68knommu.

Re: v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Joonsoo Kim]

On Mon, Apr 14, 2014 at 05:45:43PM -0700, Steven King wrote:
> git bisect suggests it starts somewhere around commit 
> f315e3fa1cf5b3317fc948708645fff889ce1e63 slab: restrict the number of objects 
> in a slab
> 
> but its kinda hard to tell as there is some compile breakage in there as well.

Hello, Steven.

Hmm... there is the fix on upstream v3.15-rc1 for build breakage.
See commit 24f870d('slab: fix wrongly used macro').
If slab allocator broken with this fix, please let me know.

Thanks.

Re: v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Steven King]

On Tuesday 15 April 2014 5:19:31 pm Joonsoo Kim wrote:
> On Mon, Apr 14, 2014 at 05:45:43PM -0700, Steven King wrote:
> > git bisect suggests it starts somewhere around commit
> > f315e3fa1cf5b3317fc948708645fff889ce1e63 slab: restrict the number of
> > objects in a slab
> >
> > but its kinda hard to tell as there is some compile breakage in there as
> > well.
>
> Hello, Steven.
>
> Hmm... there is the fix on upstream v3.15-rc1 for build breakage.
> See commit 24f870d('slab: fix wrongly used macro').
> If slab allocator broken with this fix, please let me know.
>
> Thanks.


Yes, 24f870d fixes the build breakage but the allocator is still broken (board
doesn't boot).  However, I was able to track down the exact changes that seem
to break things; in 8dcc774 'slab: introduce byte sized index for the freelist of a slab'
there are the changes to get_free_obj and set_free_obj:

-static inline unsigned int get_free_obj(struct page *page, unsigned int idx)
+static inline freelist_idx_t get_free_obj(struct page *page, unsigned char idx)
 {
-	return ((unsigned int *)page->freelist)[idx];
+	return ((freelist_idx_t *)page->freelist)[idx];
 }
 
 static inline void set_free_obj(struct page *page,
-					unsigned int idx, unsigned int val)
+					unsigned char idx, freelist_idx_t val)
 {
-	((unsigned int *)(page->freelist))[idx] = val;
+	((freelist_idx_t *)(page->freelist))[idx] = val;
 }
 
if I change idx back to unsigned int, ie:

diff --git a/mm/slab.c b/mm/slab.c
index 388cb1a..d7f9f44 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2572,13 +2572,13 @@ static void *alloc_slabmgmt(struct kmem_cache *cachep,
        return freelist;
 }

-static inline freelist_idx_t get_free_obj(struct page *page, unsigned char idx)
+static inline freelist_idx_t get_free_obj(struct page *page, unsigned int idx)
 {
        return ((freelist_idx_t *)page->freelist)[idx];
 }

 static inline void set_free_obj(struct page *page,
-                                       unsigned char idx, freelist_idx_t val)
+                                       unsigned int idx, freelist_idx_t val)
 {
        ((freelist_idx_t *)(page->freelist))[idx] = val;
 }


then v3.15-rc1 will boot using the slab allocator.

Re: [uClinux-dev] v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Geert Uytterhoeven]

Hi Steven,

On Wed, Apr 16, 2014 at 5:47 PM, Steven King <sfking@fdwdc.com> wrote:
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2572,13 +2572,13 @@ static void *alloc_slabmgmt(struct kmem_cache *cachep,
>         return freelist;
>  }
>
> -static inline freelist_idx_t get_free_obj(struct page *page, unsigned char idx)
> +static inline freelist_idx_t get_free_obj(struct page *page, unsigned int idx)
>  {
>         return ((freelist_idx_t *)page->freelist)[idx];
>  }
>
>  static inline void set_free_obj(struct page *page,
> -                                       unsigned char idx, freelist_idx_t val)
> +                                       unsigned int idx, freelist_idx_t val)
>  {
>         ((freelist_idx_t *)(page->freelist))[idx] = val;
>  }
>
>
> then v3.15-rc1 will boot using the slab allocator.

Is "idx" ever larger than 255?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [uClinux-dev] v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Steven King]

On Wednesday 16 April 2014 9:06:57 am Geert Uytterhoeven wrote:
> Hi Steven,
>
> On Wed, Apr 16, 2014 at 5:47 PM, Steven King <sfking@fdwdc.com> wrote:
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -2572,13 +2572,13 @@ static void *alloc_slabmgmt(struct kmem_cache
> > *cachep, return freelist;
> >  }
> >
> > -static inline freelist_idx_t get_free_obj(struct page *page, unsigned
> > char idx) +static inline freelist_idx_t get_free_obj(struct page *page,
> > unsigned int idx) {
> >         return ((freelist_idx_t *)page->freelist)[idx];
> >  }
> >
> >  static inline void set_free_obj(struct page *page,
> > -                                       unsigned char idx, freelist_idx_t
> > val) +                                       unsigned int idx,
> > freelist_idx_t val) {
> >         ((freelist_idx_t *)(page->freelist))[idx] = val;
> >  }
> >
> >
> > then v3.15-rc1 will boot using the slab allocator.
>
> Is "idx" ever larger than 255?
>
> Gr{oetje,eeting}s,

Yes.  If I stick

        if (idx > 255)
                pr_info("%s %d\n", __func__, idx);

in get_free_obj and set_free_obj and see values for idx up into the 400s.

Re: [uClinux-dev] v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Joonsoo Kim]

On Wed, Apr 16, 2014 at 10:44:11AM -0700, Steven King wrote:
> On Wednesday 16 April 2014 9:06:57 am Geert Uytterhoeven wrote:
> > Hi Steven,
> >
> > On Wed, Apr 16, 2014 at 5:47 PM, Steven King <sfking@fdwdc.com> wrote:
> > > --- a/mm/slab.c
> > > +++ b/mm/slab.c
> > > @@ -2572,13 +2572,13 @@ static void *alloc_slabmgmt(struct kmem_cache
> > > *cachep, return freelist;
> > >  }
> > >
> > > -static inline freelist_idx_t get_free_obj(struct page *page, unsigned
> > > char idx) +static inline freelist_idx_t get_free_obj(struct page *page,
> > > unsigned int idx) {
> > >         return ((freelist_idx_t *)page->freelist)[idx];
> > >  }
> > >
> > >  static inline void set_free_obj(struct page *page,
> > > -                                       unsigned char idx, freelist_idx_t
> > > val) +                                       unsigned int idx,
> > > freelist_idx_t val) {
> > >         ((freelist_idx_t *)(page->freelist))[idx] = val;
> > >  }
> > >
> > >
> > > then v3.15-rc1 will boot using the slab allocator.
> >
> > Is "idx" ever larger than 255?
> >
> > Gr{oetje,eeting}s,
> 
> Yes.  If I stick
> 
>         if (idx > 255)
>                 pr_info("%s %d\n", __func__, idx);
> 
> in get_free_obj and set_free_obj and see values for idx up into the 400s.

Hello,

Yes, it's my mistake. idx can be larger than 255 if freelist_idx_t is
unsigned short. So unsigned char idx isn't appropriate here. Your
system's PAGE_SIZE may be 2^13, so freelist_idx_t would be unsigned short
and idx will be larger than 255.

Your fix looks good to me, so could you send it quickly to Pekka with
some description? If you don't have enough time to do it, I can handle it.

Really thanks for notifying this issue.

Thanks.

Re: [uClinux-dev] v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Steven King]

On Wednesday 16 April 2014 6:49:11 pm Joonsoo Kim wrote:
> On Wed, Apr 16, 2014 at 10:44:11AM -0700, Steven King wrote:
> > On Wednesday 16 April 2014 9:06:57 am Geert Uytterhoeven wrote:
> > > Hi Steven,
> > >
> > > On Wed, Apr 16, 2014 at 5:47 PM, Steven King <sfking@fdwdc.com> wrote:
> > > > --- a/mm/slab.c
> > > > +++ b/mm/slab.c
> > > > @@ -2572,13 +2572,13 @@ static void *alloc_slabmgmt(struct kmem_cache
> > > > *cachep, return freelist;
> > > >  }
> > > >
> > > > -static inline freelist_idx_t get_free_obj(struct page *page,
> > > > unsigned char idx) +static inline freelist_idx_t get_free_obj(struct
> > > > page *page, unsigned int idx) {
> > > >         return ((freelist_idx_t *)page->freelist)[idx];
> > > >  }
> > > >
> > > >  static inline void set_free_obj(struct page *page,
> > > > -                                       unsigned char idx,
> > > > freelist_idx_t val) +                                       unsigned
> > > > int idx, freelist_idx_t val) {
> > > >         ((freelist_idx_t *)(page->freelist))[idx] = val;
> > > >  }
> > > >
> > > >
> > > > then v3.15-rc1 will boot using the slab allocator.
> > >
> > > Is "idx" ever larger than 255?
> > >
> > > Gr{oetje,eeting}s,
> >
> > Yes.  If I stick
> >
> >         if (idx > 255)
> >                 pr_info("%s %d\n", __func__, idx);
> >
> > in get_free_obj and set_free_obj and see values for idx up into the 400s.
>
> Hello,
>
> Yes, it's my mistake. idx can be larger than 255 if freelist_idx_t is
> unsigned short. So unsigned char idx isn't appropriate here. Your
> system's PAGE_SIZE may be 2^13, so freelist_idx_t would be unsigned short
> and idx will be larger than 255.
>
> Your fix looks good to me, so could you send it quickly to Pekka with
> some description? If you don't have enough time to do it, I can handle it.
>
> Really thanks for notifying this issue.
>
> Thanks.

Why don't you go ahead and do it, you know whats going better than I do.

Re: [uClinux-dev] v3.15-rc1 slab allocator broken on m68knommu (coldfire)

[Joonsoo Kim]

On Thu, Apr 17, 2014 at 12:09:43PM -0700, Steven King wrote:
> On Wednesday 16 April 2014 6:49:11 pm Joonsoo Kim wrote:
> > On Wed, Apr 16, 2014 at 10:44:11AM -0700, Steven King wrote:
> > > On Wednesday 16 April 2014 9:06:57 am Geert Uytterhoeven wrote:
> > > > Hi Steven,
> > > >
> > > > On Wed, Apr 16, 2014 at 5:47 PM, Steven King <sfking@fdwdc.com> wrote:
> > > > > --- a/mm/slab.c
> > > > > +++ b/mm/slab.c
> > > > > @@ -2572,13 +2572,13 @@ static void *alloc_slabmgmt(struct kmem_cache
> > > > > *cachep, return freelist;
> > > > >  }
> > > > >
> > > > > -static inline freelist_idx_t get_free_obj(struct page *page,
> > > > > unsigned char idx) +static inline freelist_idx_t get_free_obj(struct
> > > > > page *page, unsigned int idx) {
> > > > >         return ((freelist_idx_t *)page->freelist)[idx];
> > > > >  }
> > > > >
> > > > >  static inline void set_free_obj(struct page *page,
> > > > > -                                       unsigned char idx,
> > > > > freelist_idx_t val) +                                       unsigned
> > > > > int idx, freelist_idx_t val) {
> > > > >         ((freelist_idx_t *)(page->freelist))[idx] = val;
> > > > >  }
> > > > >
> > > > >
> > > > > then v3.15-rc1 will boot using the slab allocator.
> > > >
> > > > Is "idx" ever larger than 255?
> > > >
> > > > Gr{oetje,eeting}s,
> > >
> > > Yes.  If I stick
> > >
> > >         if (idx > 255)
> > >                 pr_info("%s %d\n", __func__, idx);
> > >
> > > in get_free_obj and set_free_obj and see values for idx up into the 400s.
> >
> > Hello,
> >
> > Yes, it's my mistake. idx can be larger than 255 if freelist_idx_t is
> > unsigned short. So unsigned char idx isn't appropriate here. Your
> > system's PAGE_SIZE may be 2^13, so freelist_idx_t would be unsigned short
> > and idx will be larger than 255.
> >
> > Your fix looks good to me, so could you send it quickly to Pekka with
> > some description? If you don't have enough time to do it, I can handle it.
> >
> > Really thanks for notifying this issue.
> >
> > Thanks.
> 
> Why don't you go ahead and do it, you know whats going better than I do.

Okay.

I sent it with some description.
See following link if you want to check.

https://lkml.org/lkml/2014/4/18/28

Thanks.