All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
To: Benjamin Herrenschmidt <benh-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>,
	linuxppc-dev-mnsaURCQ41sdnm+yROfE0A@public.gmane.org,
	takata-TMSiXQfHlQjsTix1lMzHGQ@public.gmane.org,
	linux-m32r-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy@public.gmane.org,
	Linux-Arch <linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
Date: Sun, 13 Apr 2008 19:13:38 -0700	[thread overview]
Message-ID: <20080413191338.9776ebd0.akpm@linux-foundation.org> (raw)
In-Reply-To: <1208133506.6958.82.camel@pasglop>

On Mon, 14 Apr 2008 10:38:26 +1000 Benjamin Herrenschmidt <benh-mnsaURCQ41sdnm+yROfE0A@public.gmane.org> wrote:

> > > +#ifndef thread_info_cache_init
> > > +#define thread_info_cache_init	do { } while(0)
> > > +#endif
> > 
> > This trick does cause a bit of a problem: it is undefined which arch header
> > file is to provide the alternative definition of thread_info_cache_init.
> 
> I this case it's well defined: thread_info.h. Maybe I should add a
> comment ?
> 
> > So we can (and have) ended up in the situation where the override appears
> > in different files on different architectures and various screwups ensue.
> 
> Yup.
> 
> > So I'd suggest that we have a bigfatcomment telling implementors which file
> > the override should be implemented in.  And make sure that this arch file is
> > directly included from within sched.h.
> 
> Will do.
> 
> > I have a suspicion that we can still get in a mess if .c files include the
> > per-arch file and don't include sched.h, but I forget where this happened
> > and why it broke stuff.
> 
> In this case, there's only one call site and will only every be one, so
> that shouldn't be a problem. I don't see init/main.c not including
> sched.h

As long as init.c directly includes sched.h, and as long as sched.h
directly includes thread_info.h and as long as all architectures which
provide the override put it in their thread_info.h, and as long as the same
applies to all future .c users, we're good.  That's a lot of "as long as"'s ;)

> > Sigh.  A nice, coded-in-C implementation within each and every architecture
> > remains the best implementation, and all the little tricks-to-save-typing
> > have failure modes.
> 
> Well, I started doing it in all arch, and people around here told me
> that was not a good idea , that it would be trouble if the prototype
> ever had to change (adding an arg, etc... though very unlikely to happen
> in that case, granted).

Bah.  Use of grep and basic typing skills: not so hard.

> > otoh, if only one .c file will ever call this function then I think that
> > all problems are solved by
> > 
> > a) moving the above ifdeffery into the .c file
> > b) adding a comment explaining which arch file must provide the override
> > c) directly including that file from within the .c file.
> 
> I can definitely do that. I have no problem either way. I can add to all
> archs too, it's just that whatever way I choose, some people won't be
> happy with it :-)
> 
> Anyway, I'll move the ifdeferry to init/main.c then.

Thanks ;)

I'm still wounded by my recent encounter with set_softirq_pending()
and or_softirq_pending().

WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org>
To: Benjamin Herrenschmidt <benh@ozlabs.org>
Cc: linux-kernel@vger.kernel.org, Paul Mackerras <paulus@samba.org>,
	linuxppc-dev@ozlabs.org, takata@linux-m32r.org,
	linux-m32r@ml.linux-m32r.org,
	Linux-Arch <linux-arch@vger.kernel.org>
Subject: Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
Date: Sun, 13 Apr 2008 19:13:38 -0700	[thread overview]
Message-ID: <20080413191338.9776ebd0.akpm@linux-foundation.org> (raw)
Message-ID: <20080414021338.fW81fR-wTdkXia8AG-e-IicMLMuzdFdiNX9F-vybYwQ@z> (raw)
In-Reply-To: <1208133506.6958.82.camel@pasglop>

On Mon, 14 Apr 2008 10:38:26 +1000 Benjamin Herrenschmidt <benh@ozlabs.org> wrote:

> > > +#ifndef thread_info_cache_init
> > > +#define thread_info_cache_init	do { } while(0)
> > > +#endif
> > 
> > This trick does cause a bit of a problem: it is undefined which arch header
> > file is to provide the alternative definition of thread_info_cache_init.
> 
> I this case it's well defined: thread_info.h. Maybe I should add a
> comment ?
> 
> > So we can (and have) ended up in the situation where the override appears
> > in different files on different architectures and various screwups ensue.
> 
> Yup.
> 
> > So I'd suggest that we have a bigfatcomment telling implementors which file
> > the override should be implemented in.  And make sure that this arch file is
> > directly included from within sched.h.
> 
> Will do.
> 
> > I have a suspicion that we can still get in a mess if .c files include the
> > per-arch file and don't include sched.h, but I forget where this happened
> > and why it broke stuff.
> 
> In this case, there's only one call site and will only every be one, so
> that shouldn't be a problem. I don't see init/main.c not including
> sched.h

As long as init.c directly includes sched.h, and as long as sched.h
directly includes thread_info.h and as long as all architectures which
provide the override put it in their thread_info.h, and as long as the same
applies to all future .c users, we're good.  That's a lot of "as long as"'s ;)

> > Sigh.  A nice, coded-in-C implementation within each and every architecture
> > remains the best implementation, and all the little tricks-to-save-typing
> > have failure modes.
> 
> Well, I started doing it in all arch, and people around here told me
> that was not a good idea , that it would be trouble if the prototype
> ever had to change (adding an arg, etc... though very unlikely to happen
> in that case, granted).

Bah.  Use of grep and basic typing skills: not so hard.

> > otoh, if only one .c file will ever call this function then I think that
> > all problems are solved by
> > 
> > a) moving the above ifdeffery into the .c file
> > b) adding a comment explaining which arch file must provide the override
> > c) directly including that file from within the .c file.
> 
> I can definitely do that. I have no problem either way. I can add to all
> archs too, it's just that whatever way I choose, some people won't be
> happy with it :-)
> 
> Anyway, I'll move the ifdeferry to init/main.c then.

Thanks ;)

I'm still wounded by my recent encounter with set_softirq_pending()
and or_softirq_pending().

WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org>
To: Benjamin Herrenschmidt <benh@ozlabs.org>
Cc: Linux-Arch <linux-arch@vger.kernel.org>,
	linux-m32r@ml.linux-m32r.org, takata@linux-m32r.org,
	linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org,
	Paul Mackerras <paulus@samba.org>
Subject: Re: [PATCH 1/2] Add thread_info_cache_init() to all archs
Date: Sun, 13 Apr 2008 19:13:38 -0700	[thread overview]
Message-ID: <20080413191338.9776ebd0.akpm@linux-foundation.org> (raw)
In-Reply-To: <1208133506.6958.82.camel@pasglop>

On Mon, 14 Apr 2008 10:38:26 +1000 Benjamin Herrenschmidt <benh@ozlabs.org> wrote:

> > > +#ifndef thread_info_cache_init
> > > +#define thread_info_cache_init	do { } while(0)
> > > +#endif
> > 
> > This trick does cause a bit of a problem: it is undefined which arch header
> > file is to provide the alternative definition of thread_info_cache_init.
> 
> I this case it's well defined: thread_info.h. Maybe I should add a
> comment ?
> 
> > So we can (and have) ended up in the situation where the override appears
> > in different files on different architectures and various screwups ensue.
> 
> Yup.
> 
> > So I'd suggest that we have a bigfatcomment telling implementors which file
> > the override should be implemented in.  And make sure that this arch file is
> > directly included from within sched.h.
> 
> Will do.
> 
> > I have a suspicion that we can still get in a mess if .c files include the
> > per-arch file and don't include sched.h, but I forget where this happened
> > and why it broke stuff.
> 
> In this case, there's only one call site and will only every be one, so
> that shouldn't be a problem. I don't see init/main.c not including
> sched.h

As long as init.c directly includes sched.h, and as long as sched.h
directly includes thread_info.h and as long as all architectures which
provide the override put it in their thread_info.h, and as long as the same
applies to all future .c users, we're good.  That's a lot of "as long as"'s ;)

> > Sigh.  A nice, coded-in-C implementation within each and every architecture
> > remains the best implementation, and all the little tricks-to-save-typing
> > have failure modes.
> 
> Well, I started doing it in all arch, and people around here told me
> that was not a good idea , that it would be trouble if the prototype
> ever had to change (adding an arg, etc... though very unlikely to happen
> in that case, granted).

Bah.  Use of grep and basic typing skills: not so hard.

> > otoh, if only one .c file will ever call this function then I think that
> > all problems are solved by
> > 
> > a) moving the above ifdeffery into the .c file
> > b) adding a comment explaining which arch file must provide the override
> > c) directly including that file from within the .c file.
> 
> I can definitely do that. I have no problem either way. I can add to all
> archs too, it's just that whatever way I choose, some people won't be
> happy with it :-)
> 
> Anyway, I'll move the ifdeferry to init/main.c then.

Thanks ;)

I'm still wounded by my recent encounter with set_softirq_pending()
and or_softirq_pending().

  reply	other threads:[~2008-04-14  2:13 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-04-10  3:22 [PATCH 1/2] Add thread_info_cache_init() to all archs Benjamin Herrenschmidt
2008-04-10  3:22 ` Benjamin Herrenschmidt
2008-04-10  3:22 ` Benjamin Herrenschmidt
2008-04-10  3:22 ` Benjamin Herrenschmidt
     [not found] ` <20080410032354.90CB1DDF0F-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
2008-04-10 21:46   ` Benjamin Herrenschmidt
2008-04-10 21:46     ` Benjamin Herrenschmidt
2008-04-14  0:19   ` Andrew Morton
2008-04-14  0:19     ` Andrew Morton
2008-04-14  0:19     ` Andrew Morton
2008-04-14  0:19     ` Andrew Morton
2008-04-14  0:38     ` Benjamin Herrenschmidt
2008-04-14  0:38       ` Benjamin Herrenschmidt
2008-04-14  2:13       ` Andrew Morton [this message]
2008-04-14  2:13         ` Andrew Morton
2008-04-14  2:13         ` Andrew Morton
     [not found]         ` <20080413191338.9776ebd0.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
2008-04-18  3:58           ` Benjamin Herrenschmidt
2008-04-18  3:58             ` Benjamin Herrenschmidt
2008-04-18  3:58             ` Benjamin Herrenschmidt
2008-04-18  4:19             ` Andrew Morton
2008-04-18  4:19               ` Andrew Morton
2008-04-18  4:19               ` Andrew Morton
     [not found]               ` <20080417211905.8ff769fa.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
2008-04-18  4:38                 ` Michael Ellerman
2008-04-18  4:38                   ` Michael Ellerman
2008-04-18  6:44                 ` Benjamin Herrenschmidt
2008-04-18  6:44                   ` Benjamin Herrenschmidt
2008-04-18  6:44                   ` Benjamin Herrenschmidt
2008-05-21 17:56                   ` Benjamin Herrenschmidt
2008-05-21 17:56                     ` Benjamin Herrenschmidt
2008-05-21 18:41                     ` Andrew Morton
2008-05-21 18:41                       ` Andrew Morton
2008-05-21 19:06                       ` Sam Ravnborg
2008-05-21 19:06                         ` Sam Ravnborg
2008-05-21 20:38                         ` Benjamin Herrenschmidt
2008-05-21 20:38                           ` Benjamin Herrenschmidt
2008-05-21 19:44                       ` Benjamin Herrenschmidt
2008-05-21 19:44                         ` Benjamin Herrenschmidt
2008-05-21 20:52                         ` Andrew Morton
2008-05-21 20:52                           ` Andrew Morton
2008-04-18  4:21             ` Kyle McMartin
2008-04-18  4:21               ` Kyle McMartin
  -- strict thread matches above, loose matches on Subject: below --
2008-04-10 23:36 Benjamin Herrenschmidt
2008-04-10 23:36 ` Benjamin Herrenschmidt
2008-04-10 23:36 ` Benjamin Herrenschmidt
2008-04-10 23:36 ` Benjamin Herrenschmidt

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=20080413191338.9776ebd0.akpm@linux-foundation.org \
    --to=akpm-de/tnxtf+jlsfhdxvbkv3wd2fqjk+8+b@public.gmane.org \
    --cc=benh-mnsaURCQ41sdnm+yROfE0A@public.gmane.org \
    --cc=linux-arch-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-m32r-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy@public.gmane.org \
    --cc=linuxppc-dev-mnsaURCQ41sdnm+yROfE0A@public.gmane.org \
    --cc=paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org \
    --cc=takata-TMSiXQfHlQjsTix1lMzHGQ@public.gmane.org \
    /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.