Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1210787 > unrolled thread

Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one word in struct page

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.

Contents

  Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into  one word in struct page Andrew Morton <akpm@linux-foundation.org> - 2015-08-21 01:30 +0200
    Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one  word in struct page Michal Hocko <mhocko@kernel.org> - 2015-08-21 09:20 +0200
      Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one  word in struct page "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-21 12:50 +0200
        Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one  word in struct page Michal Hocko <mhocko@kernel.org> - 2015-08-21 13:00 +0200

#1210787 — Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one word in struct page

On Wed, 19 Aug 2015 12:21:44 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> The patch halves space occupied by compound_dtor and compound_order in
> struct page.
> 
> For compound_order, it's trivial long -> int/short conversion.
> 
> For get_compound_page_dtor(), we now use hardcoded table for destructor
> lookup and store its index in the struct page instead of direct pointer
> to destructor. It shouldn't be a big trouble to maintain the table: we
> have only two destructor and NULL currently.
> 
> This patch free up one word in tail pages for reuse. This is preparation
> for the next patch.
> 
> ...
>
> @@ -145,8 +143,13 @@ struct page {
>  						 */
>  		/* First tail page of compound page */
>  		struct {
> -			compound_page_dtor *compound_dtor;
> -			unsigned long compound_order;
> +#ifdef CONFIG_64BIT
> +			unsigned int compound_dtor;
> +			unsigned int compound_order;
> +#else
> +			unsigned short int compound_dtor;
> +			unsigned short int compound_order;
> +#endif

Why not use ushort for 64-bit as well?

It would be clearer if that new enum had a name, so we use "enum
compound_dtor_id" everywhere instead of a bare uint.

--
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]

#1210955 — Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one word in struct page

On Thu 20-08-15 16:26:04, Andrew Morton wrote:
> On Wed, 19 Aug 2015 12:21:44 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > The patch halves space occupied by compound_dtor and compound_order in
> > struct page.
> > 
> > For compound_order, it's trivial long -> int/short conversion.
> > 
> > For get_compound_page_dtor(), we now use hardcoded table for destructor
> > lookup and store its index in the struct page instead of direct pointer
> > to destructor. It shouldn't be a big trouble to maintain the table: we
> > have only two destructor and NULL currently.
> > 
> > This patch free up one word in tail pages for reuse. This is preparation
> > for the next patch.
> > 
> > ...
> >
> > @@ -145,8 +143,13 @@ struct page {
> >  						 */
> >  		/* First tail page of compound page */
> >  		struct {
> > -			compound_page_dtor *compound_dtor;
> > -			unsigned long compound_order;
> > +#ifdef CONFIG_64BIT
> > +			unsigned int compound_dtor;
> > +			unsigned int compound_order;
> > +#else
> > +			unsigned short int compound_dtor;
> > +			unsigned short int compound_order;
> > +#endif
> 
> Why not use ushort for 64-bit as well?

Yeah, I have asked the same in the previous round. So I've tried to
compile with ushort. The resulting code was slightly larger
   text    data     bss     dec     hex filename
 476370   90811   44632  611813   955e5 mm/built-in.o.prev
 476418   90811   44632  611861   95615 mm/built-in.o.after

E.g. prep_compound_page
before:
4c6b:       c7 47 68 01 00 00 00    movl   $0x1,0x68(%rdi)
4c72:       89 77 6c                mov    %esi,0x6c(%rdi)
after:
4c6c:       66 c7 47 68 01 00       movw   $0x1,0x68(%rdi)
4c72:       66 89 77 6a             mov    %si,0x6a(%rdi)

which looks very similar to me but I am not an expert here so it might
possible that movw is slower.

__free_pages_ok
before:
63af:       8b 77 6c                mov    0x6c(%rdi),%esi
after:
63b1:       0f b7 77 6a             movzwl 0x6a(%rdi),%esi

which looks like a worse code to me. Whether this all is measurable or
worth it I dunno. The ifdef is ugly but maybe the ugliness is a destiny
for struct page.
-- 
Michal Hocko
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]

#1211115 — Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one word in struct page

On Fri, Aug 21, 2015 at 09:13:42AM +0200, Michal Hocko wrote:
> On Thu 20-08-15 16:26:04, Andrew Morton wrote:
> > On Wed, 19 Aug 2015 12:21:44 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > 
> > > The patch halves space occupied by compound_dtor and compound_order in
> > > struct page.
> > > 
> > > For compound_order, it's trivial long -> int/short conversion.
> > > 
> > > For get_compound_page_dtor(), we now use hardcoded table for destructor
> > > lookup and store its index in the struct page instead of direct pointer
> > > to destructor. It shouldn't be a big trouble to maintain the table: we
> > > have only two destructor and NULL currently.
> > > 
> > > This patch free up one word in tail pages for reuse. This is preparation
> > > for the next patch.
> > > 
> > > ...
> > >
> > > @@ -145,8 +143,13 @@ struct page {
> > >  						 */
> > >  		/* First tail page of compound page */
> > >  		struct {
> > > -			compound_page_dtor *compound_dtor;
> > > -			unsigned long compound_order;
> > > +#ifdef CONFIG_64BIT
> > > +			unsigned int compound_dtor;
> > > +			unsigned int compound_order;
> > > +#else
> > > +			unsigned short int compound_dtor;
> > > +			unsigned short int compound_order;
> > > +#endif
> > 
> > Why not use ushort for 64-bit as well?
> 
> Yeah, I have asked the same in the previous round. So I've tried to
> compile with ushort. The resulting code was slightly larger
>    text    data     bss     dec     hex filename
>  476370   90811   44632  611813   955e5 mm/built-in.o.prev
>  476418   90811   44632  611861   95615 mm/built-in.o.after
> 
> E.g. prep_compound_page
> before:
> 4c6b:       c7 47 68 01 00 00 00    movl   $0x1,0x68(%rdi)
> 4c72:       89 77 6c                mov    %esi,0x6c(%rdi)
> after:
> 4c6c:       66 c7 47 68 01 00       movw   $0x1,0x68(%rdi)
> 4c72:       66 89 77 6a             mov    %si,0x6a(%rdi)
> 
> which looks very similar to me but I am not an expert here so it might
> possible that movw is slower.
> 
> __free_pages_ok
> before:
> 63af:       8b 77 6c                mov    0x6c(%rdi),%esi
> after:
> 63b1:       0f b7 77 6a             movzwl 0x6a(%rdi),%esi
> 
> which looks like a worse code to me. Whether this all is measurable or
> worth it I dunno. The ifdef is ugly but maybe the ugliness is a destiny
> for struct page.

I don't care about the ifdef that much. If you guys prefer to drop it I'm
fine with that.

-- 
 Kirill A. Shutemov
--
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]

#1211116 — Re: [PATCHv3 3/5] mm: pack compound_dtor and compound_order into one word in struct page

On Fri 21-08-15 13:40:59, Kirill A. Shutemov wrote:
> On Fri, Aug 21, 2015 at 09:13:42AM +0200, Michal Hocko wrote:
> > On Thu 20-08-15 16:26:04, Andrew Morton wrote:
[...]
> > > Why not use ushort for 64-bit as well?
> > 
> > Yeah, I have asked the same in the previous round. So I've tried to
> > compile with ushort. The resulting code was slightly larger
> >    text    data     bss     dec     hex filename
> >  476370   90811   44632  611813   955e5 mm/built-in.o.prev
> >  476418   90811   44632  611861   95615 mm/built-in.o.after
> > 
> > E.g. prep_compound_page
> > before:
> > 4c6b:       c7 47 68 01 00 00 00    movl   $0x1,0x68(%rdi)
> > 4c72:       89 77 6c                mov    %esi,0x6c(%rdi)
> > after:
> > 4c6c:       66 c7 47 68 01 00       movw   $0x1,0x68(%rdi)
> > 4c72:       66 89 77 6a             mov    %si,0x6a(%rdi)
> > 
> > which looks very similar to me but I am not an expert here so it might
> > possible that movw is slower.
> > 
> > __free_pages_ok
> > before:
> > 63af:       8b 77 6c                mov    0x6c(%rdi),%esi
> > after:
> > 63b1:       0f b7 77 6a             movzwl 0x6a(%rdi),%esi
> > 
> > which looks like a worse code to me. Whether this all is measurable or
> > worth it I dunno. The ifdef is ugly but maybe the ugliness is a destiny
> > for struct page.
> 
> I don't care about the ifdef that much. If you guys prefer to drop it I'm
> fine with that.

I can live with it. It makes the struct more complicated which is what
struck me. If there is a good reason and a better generated code is a
good one then I do not object but please make it a separate patch so
that we do not wonder why this has been done in the future.
-- 
Michal Hocko
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web