public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Robert Jarzmik <robert.jarzmik@free.fr>
Cc: Dave Martin <Dave.Martin@arm.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] ARM: fix alignement of __bug_table section entries
Date: Fri, 11 Sep 2015 10:54:38 +0100	[thread overview]
Message-ID: <20150911095438.GL21084@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <8737ymuhbc.fsf@belgarion.home>

On Thu, Sep 10, 2015 at 10:53:43PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> > I've been wondering whether we can teach GCC that set_domain modifies
> > the value that get_domain returns, rather than throwing a volatile
> > onto the asm in get_domain.  The issue with a volatile there is that
> > even if the result is unused, but the code is reachable, gcc still has
> > to output the code to read the register.
> >
> > We might be able to get away with a memory clobber on the set_domain,
> > and fake a memory read in get_domain, eg, by passing
> > 	"m" (current_thread_info()->cpu_domain))
> > to the get_domain asm.
> Ok, let's say we do it that way.
> 
> I have some concerns about it:
>   (a) I see an inbalance, as set_domain() doesn't actually modify
>       current_thread_info()->cpu_domain. I don't see how it will protect use
>       from this scenario :
>         - get_domain()
>         - set_domain()
>         - set_domain()

That should be fine, because if you've only got one get_domain(), then
you only get the value of the DACR once.

>   (b) domain.h is included from thread_info.h, not the other way around
>       => current_thread_info() is not accessible from domain.h
>       => that would require a bit of moving things around, as thread_info
>          structure description should be available for example.
>       This is currently my biggest problem with this approach.

It's not a problem since 1eef5d2f1b46 removed the need for domain.h to be
included by thread_info.h - the existing include can be dropped.

>   (c) I was also wondering if a case like this could happen :
>      - a function foo() does a get_domain()
>        => an exception/irq whatever happens and modifies the DACR

We always preserve the value of DACR across an exception.

>      - foo() continues a makes a modify_domain()
>        => and here the modify_domain() uses the old DACR value
>       Or said differently, I wonder if there is a case of 2 get_domain() calls
>       in a row with a DACR modification in between. I
> 
> What about something such as [1], without a memory clobber, but a "fake" memory
> variable link ?

The problem is the compiler will need to issue instructions to arrange
for the address of this variable to end up in registers even though the
assembly doesn't use it.

That's true of my suggestion as well, but looking at the callsites, we
generally already have, or very shortly there-after have the current
thread_info address in a register.

Patches to follow - I've not been able to confirm the instruction ordering
you've observed with my compiler, so I can't prove whether this solves
the problem locally.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

  reply	other threads:[~2015-09-11  9:54 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-02  6:23 [PATCH] ARM: fix alignement of __bug_table section entries Robert Jarzmik
2015-09-02 10:39 ` Dave Martin
2015-09-05 13:48   ` Robert Jarzmik
2015-09-05 14:25     ` Russell King - ARM Linux
2015-09-05 17:10       ` Robert Jarzmik
2015-09-05 20:38         ` Russell King - ARM Linux
2015-09-05 22:12           ` Robert Jarzmik
2015-09-06 17:25           ` Robert Jarzmik
2015-09-06 19:48             ` Russell King - ARM Linux
2015-09-06 21:31               ` Robert Jarzmik
2015-09-06 23:54                 ` Russell King - ARM Linux
2015-09-08 17:01                   ` Robert Jarzmik
2015-09-08 20:08                     ` Russell King - ARM Linux
2015-09-08 20:46                       ` Robert Jarzmik
2015-09-09 23:06                       ` Robert Jarzmik
2015-09-10 19:01                         ` Robert Jarzmik
2015-09-10 19:16                           ` Russell King - ARM Linux
2015-09-10 20:53                             ` Robert Jarzmik
2015-09-11  9:54                               ` Russell King - ARM Linux [this message]
2015-09-11  9:56                                 ` [PATCH 1/2] ARM: domains: thread_info.h no longer needs asm/domains.h Russell King
2015-09-11  9:56                                 ` [PATCH 2/2] ARM: domains: add memory dependencies to get_domain/set_domain Russell King
2015-09-11 14:56                                   ` Robert Jarzmik
2015-09-11 15:10                                     ` Russell King - ARM Linux
2015-09-11 15:40                                       ` Robert Jarzmik

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=20150911095438.GL21084@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=Dave.Martin@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robert.jarzmik@free.fr \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox