linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-unionfs@vger.kernel.org
Subject: Re: [GIT PULL] overlayfs update for 4.15
Date: Fri, 17 Nov 2017 13:49:01 -0800	[thread overview]
Message-ID: <CA+55aFxPPXo1UD_4kuSamJYx1FbcpVFx3ANzfxJpFKVhd_gROQ@mail.gmail.com> (raw)
In-Reply-To: <20171117151317.GA5348@veci.piliscsaba.szeredi.hu>

On Fri, Nov 17, 2017 at 7:13 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> Created a path_put_init() helper that clears out the pointers after putting the
> ref.  I think this could be useful elsewhere, so added it to <linux/path.h>.

Slight eww.

The problem with your helper is that we've seen gcc generate really
horrible code for things like that.

So when you do

     *path = (struct path) { };

we've seen gcc first create an local empty "struct path" on stack, and
then memcpy() it over the target. Which is _technically_ what that
code does, of course, but it's also excessively stupid.

So I suspect that would be better off as just

     memset(path, 0, sizeof(*path));

which then matches the code that you actually would expect gcc to generate.

I hope that "struct path" is small enough that gcc doesn't mess up,
and that odd code generation is probably specific to some gcc versions
anyway, but we've definitely seen this.

NOTE! The above pattern of assignment is very different from the
initialization pattern. Gcc generally does well on structure
initializers:

    struct xyz a = { .. };

generally generates reasonable code in ways that

    struct xyz a;
    ..
    a = (struct xyz) { ...};

sometimes doesn't.  I suspect it's mainly a "initializers are common,
unnamed temporary local structures are not" thing.

                  Linus

      reply	other threads:[~2017-11-17 21:49 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-17 15:13 [GIT PULL] overlayfs update for 4.15 Miklos Szeredi
2017-11-17 21:49 ` Linus Torvalds [this message]

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=CA+55aFxPPXo1UD_4kuSamJYx1FbcpVFx3ANzfxJpFKVhd_gROQ@mail.gmail.com \
    --to=torvalds@linux-foundation.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    /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;
as well as URLs for NNTP newsgroup(s).