All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: David Daney <ddaney@caviumnetworks.com>
Cc: Jason Baron <jbaron@akamai.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-mips@linux-mips.org,
	linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@kernel.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Anton Blanchard <anton@samba.org>, Rabin Vincent <rabin@rab.in>,
	Russell King <linux@armlinux.org.uk>,
	Ralf Baechle <ralf@linux-mips.org>,
	Chris Metcalf <cmetcalf@mellanox.com>,
	Zhigang Lu <zlu@ezchip.com>
Subject: Re: [PATCH] jump_label: align jump_entry table to at least 4-bytes
Date: Mon, 27 Feb 2017 16:06:01 -0500	[thread overview]
Message-ID: <20170227160601.5b79a1fe@gandalf.local.home> (raw)
In-Reply-To: <aa139c18-1b04-2c20-2e22-89d74503b3cf@caviumnetworks.com>

On Mon, 27 Feb 2017 11:59:50 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> For me the size is not the important issue, it is the alignment of the 
> struct jump_entry entries in the table.  I don't understand how your 
> patch helps, and I cannot Acked-by unless I understand what is being 
> done and can see that it is both correct and necessary.

You brought up a very good point and I'm glad that I had Jason Cc all
the arch maintainers in one patch.

I think jump_labels may be much more broken than we think, and Jason's
fix doesn't fix anything. We had this same issues with tracepoints.

I'm looking at jump_label_init, and how we iterate over an array of
struct jump_entry's that was put together by the linker. The problem is
that jump_entry is not a power of 2 in size.

struct jump_entry {
	jump_label_t code;
	jump_label_t target;
	jump_label_t key;
};

When putting together arrays of this kind, the linker is in its right
to add padding for alignment, in the middle of the array! It has no
idea that this is an array, and there's nothing stopping the linker
from messing it up.

For those structs that are a power of 2 in size, there's no reason for
the linker to do anything else, and it "just works". There's plenty of
instances in the kernel that depend on this.

I'm thinking that the sort algorithm either hid the problem or fixed it
somehow (I'm guessing it hid the problem).

I hit the same issue with trace event structures. The solution was to
create the array of pointers to each structure, and dereference the
structures from the array.

See commit e4a9ea5ee ("tracing: Replace trace_event struct array with
pointer array")

-- Steve

WARNING: multiple messages have this Message-ID (diff)
From: rostedt@goodmis.org (Steven Rostedt)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] jump_label: align jump_entry table to at least 4-bytes
Date: Mon, 27 Feb 2017 16:06:01 -0500	[thread overview]
Message-ID: <20170227160601.5b79a1fe@gandalf.local.home> (raw)
In-Reply-To: <aa139c18-1b04-2c20-2e22-89d74503b3cf@caviumnetworks.com>

On Mon, 27 Feb 2017 11:59:50 -0800
David Daney <ddaney@caviumnetworks.com> wrote:

> For me the size is not the important issue, it is the alignment of the 
> struct jump_entry entries in the table.  I don't understand how your 
> patch helps, and I cannot Acked-by unless I understand what is being 
> done and can see that it is both correct and necessary.

You brought up a very good point and I'm glad that I had Jason Cc all
the arch maintainers in one patch.

I think jump_labels may be much more broken than we think, and Jason's
fix doesn't fix anything. We had this same issues with tracepoints.

I'm looking at jump_label_init, and how we iterate over an array of
struct jump_entry's that was put together by the linker. The problem is
that jump_entry is not a power of 2 in size.

struct jump_entry {
	jump_label_t code;
	jump_label_t target;
	jump_label_t key;
};

When putting together arrays of this kind, the linker is in its right
to add padding for alignment, in the middle of the array! It has no
idea that this is an array, and there's nothing stopping the linker
from messing it up.

For those structs that are a power of 2 in size, there's no reason for
the linker to do anything else, and it "just works". There's plenty of
instances in the kernel that depend on this.

I'm thinking that the sort algorithm either hid the problem or fixed it
somehow (I'm guessing it hid the problem).

I hit the same issue with trace event structures. The solution was to
create the array of pointers to each structure, and dereference the
structures from the array.

See commit e4a9ea5ee ("tracing: Replace trace_event struct array with
pointer array")

-- Steve

  reply	other threads:[~2017-02-28  2:31 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-27 18:49 [PATCH] jump_label: align jump_entry table to at least 4-bytes Jason Baron
2017-02-27 18:49 ` Jason Baron
2017-02-27 18:57 ` David Daney
2017-02-27 18:57   ` David Daney
2017-02-27 19:18   ` Jason Baron
2017-02-27 19:18     ` Jason Baron
2017-02-27 19:59     ` David Daney
2017-02-27 19:59       ` David Daney
2017-02-27 21:06       ` Steven Rostedt [this message]
2017-02-27 21:06         ` Steven Rostedt
2017-02-27 21:41         ` David Daney
2017-02-27 21:41           ` David Daney
2017-02-27 22:09           ` Steven Rostedt
2017-02-27 22:09             ` Steven Rostedt
2017-02-27 22:21             ` David Daney
2017-02-27 22:21               ` David Daney
2017-02-27 22:21               ` David Daney
2017-02-27 22:36               ` Steven Rostedt
2017-02-27 22:36                 ` Steven Rostedt
2017-02-27 22:45                 ` David Daney
2017-02-27 22:45                   ` David Daney
2017-02-27 22:50                   ` Jason Baron
2017-02-27 22:50                     ` Jason Baron
2017-02-27 23:34                     ` David Daney
2017-02-27 23:34                       ` David Daney
2017-02-28  4:55                     ` Sachin Sant
2017-02-28 16:21                       ` Steven Rostedt
2017-02-28 16:21                         ` Steven Rostedt
2017-02-28 18:16                         ` David Daney
2017-02-28 18:16                           ` David Daney
2017-02-28 18:39                           ` Jason Baron
2017-02-28 18:39                             ` Jason Baron
2017-02-28 19:05                             ` David Daney
2017-02-28 19:05                               ` David Daney
2017-02-28 19:22                               ` David Daney
2017-02-28 19:22                                 ` David Daney
2017-02-28 19:34                                 ` Jason Baron
2017-02-28 19:34                                   ` Jason Baron
2017-02-28 20:15                                   ` David Daney
2017-02-28 20:15                                     ` David Daney
2017-02-28 20:15                                     ` David Daney
2017-02-28 22:41                                     ` Jason Baron
2017-02-28 22:41                                       ` Jason Baron
2017-03-01  6:34                                       ` Michael Ellerman
2017-03-01  6:34                                         ` Michael Ellerman
2017-03-01 16:40                                         ` David Daney
2017-03-01 16:40                                           ` David Daney
2017-03-01 20:02                                           ` Jason Baron
2017-03-01 20:02                                             ` Jason Baron
2017-03-01 21:12                                             ` David Daney
2017-03-01 21:12                                               ` David Daney
2017-03-01  7:12                                       ` Sachin Sant
2017-03-01  7:12                                         ` Sachin Sant
2017-02-27 22:58                   ` Steven Rostedt
2017-02-27 22:58                     ` Steven Rostedt
2017-03-06  2:16               ` [PATCH] MIPS: jump_lable: Give __jump_table elements an entsize kbuild test robot
2017-03-06  2:16                 ` kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170227160601.5b79a1fe@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --cc=anton@samba.org \
    --cc=benh@kernel.crashing.org \
    --cc=cmetcalf@mellanox.com \
    --cc=ddaney@caviumnetworks.com \
    --cc=jbaron@akamai.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    --cc=linux@armlinux.org.uk \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mingo@kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=paulus@samba.org \
    --cc=rabin@rab.in \
    --cc=ralf@linux-mips.org \
    --cc=zlu@ezchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.