Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Mikael Pettersson]

On Wed, 14 Jul 2004 23:12:54 -0700, William Lee Irwin III wrote:
>Jeff Garzik <jgarzik@pobox.com> wrote:
>>>> Or you could just call it "gcc is dumb" rather than a compiler bug.
>
>On Wed, Jul 14 2004, Andrew Morton wrote:
>[... code snippet ...]
>>> is pretty dumb too.  I don't see any harm if this compiler feature/problem
>>> pushes us to fix the above in the obvious way.
>
>On Thu, Jul 15, 2004 at 07:56:56AM +0200, Jens Axboe wrote:
>> Excuse my ignorance, but why on earth would that be dumb? Looks
>> perfectly legit to me, and I have to agree with Jeff that the compiler
>> is exceedingly dumb if it fails to inline that case.
>
>Enter gcc...
>
>Maybe "the obvious way" is sending a someone off to whip gcc into shape,
>or possibly reporting it as a gcc problem.

It shows you guys aren't compiler writers.

Compilers for top-down (define-before-use) languages like C
have traditionally also worked in a top-down fashion, processing
one top-level declaration at a time. Forward references are
either errors, or are (when a proper declaration is in scope)
left to the linker to resolve.

Processing an entire compilation-unit (e.g. whole C file)
as a single unit is typically _only_ done when either the
language semantics requires it (not C, but e.g. Haskell),
or when very high optimisation levels are requested.

In the case of gcc-3.4.1 failing to inline, you are asking
gcc to do something (peeking forward) which it never has
promised to do. And with the kernel using -fno-unit-at-a-time
for stack conservation reasons, gcc is actually being _told_
not to do global compilation.

This is not a gcc bug, nor is it being "exceedingly dumb".

/Mikael

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Mikael Pettersson wrote:
> On Wed, 14 Jul 2004 23:12:54 -0700, William Lee Irwin III wrote:
> 
>>Jeff Garzik <jgarzik@pobox.com> wrote:
>>
>>>>>Or you could just call it "gcc is dumb" rather than a compiler bug.
>>
>>On Wed, Jul 14 2004, Andrew Morton wrote:
>>[... code snippet ...]
>>
>>>>is pretty dumb too.  I don't see any harm if this compiler feature/problem
>>>>pushes us to fix the above in the obvious way.
>>
>>On Thu, Jul 15, 2004 at 07:56:56AM +0200, Jens Axboe wrote:
>>
>>>Excuse my ignorance, but why on earth would that be dumb? Looks
>>>perfectly legit to me, and I have to agree with Jeff that the compiler
>>>is exceedingly dumb if it fails to inline that case.
>>
>>Enter gcc...
>>
>>Maybe "the obvious way" is sending a someone off to whip gcc into shape,
>>or possibly reporting it as a gcc problem.
> 
> 
> It shows you guys aren't compiler writers.

Actually, I have written most of a [simple] compiler backend.


> Compilers for top-down (define-before-use) languages like C
> have traditionally also worked in a top-down fashion, processing
> one top-level declaration at a time. Forward references are
> either errors, or are (when a proper declaration is in scope)
> left to the linker to resolve.
> 
> Processing an entire compilation-unit (e.g. whole C file)
> as a single unit is typically _only_ done when either the
> language semantics requires it (not C, but e.g. Haskell),
> or when very high optimisation levels are requested.

Or in the case where you parse the entire file, then generate code for 
the entire file in a separate pass.  Which does NOT imply 
unit-at-a-time, for the readers at home.  It just implies generation of 
the AST.


> In the case of gcc-3.4.1 failing to inline, you are asking
> gcc to do something (peeking forward) which it never has
> promised to do. And with the kernel using -fno-unit-at-a-time
> for stack conservation reasons, gcc is actually being _told_
> not to do global compilation.
> 
> This is not a gcc bug, nor is it being "exceedingly dumb".

Actually, yes it is.

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Matthew Wilcox]

On Thu, Jul 15, 2004 at 09:09:38AM -0400, Jeff Garzik wrote:
> >Compilers for top-down (define-before-use) languages like C
> >have traditionally also worked in a top-down fashion, processing
> >one top-level declaration at a time. Forward references are
> >either errors, or are (when a proper declaration is in scope)
> >left to the linker to resolve.
> >
> >Processing an entire compilation-unit (e.g. whole C file)
> >as a single unit is typically _only_ done when either the
> >language semantics requires it (not C, but e.g. Haskell),
> >or when very high optimisation levels are requested.
> 
> Or in the case where you parse the entire file, then generate code for 
> the entire file in a separate pass.  Which does NOT imply 
> unit-at-a-time, for the readers at home.  It just implies generation of 
> the AST.

... which GCC didn't use to do.  It used to generate RTL directly from
the source.  From the GCC news and announcements page:

October 5, 2001
    Alexandre Oliva of Red Hat has generalized the tree inlining
    infrastructure, formerly in the C++ front end, so that it is now
    used in the C front end too.

> >In the case of gcc-3.4.1 failing to inline, you are asking
> >gcc to do something (peeking forward) which it never has
> >promised to do. And with the kernel using -fno-unit-at-a-time
> >for stack conservation reasons, gcc is actually being _told_
> >not to do global compilation.
> >
> >This is not a gcc bug, nor is it being "exceedingly dumb".
> 
> Actually, yes it is.

No, it's not.  It's always been this way.  You seemed to ignore that
message I posted yesterday.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[William Lee Irwin III]

On Thu, Jul 15, 2004 at 11:46:52AM +0200, Mikael Pettersson wrote:
> It shows you guys aren't compiler writers.
[... condescending bullshit ...]

Tons of showstopping quality of implementation issues plus copyright
assignment bullshit are why so many are writing from scratch.


-- wli

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Mikael Pettersson]

On Wed, 14 Jul 2004 11:59:01 -0400, Jeff Garzik wrote:
>Bartlomiej Zolnierkiewicz wrote:
>> Hi,
>> 
>> On Wednesday 14 of July 2004 14:16, Mikael Pettersson wrote:
>> 
>>>gcc-3.4.1 errors out in 2.6.8-rc1-mm1 at drivers/scsi/sg.c:
>>>
>>>drivers/scsi/sg.c: In function `sg_ioctl':
>>>drivers/scsi/sg.c:209: sorry, unimplemented: inlining failed in call to
>>>'sg_jif_to_ms': function body not available drivers/scsi/sg.c:930: sorry,
>>>unimplemented: called from here
>>>make[2]: *** [drivers/scsi/sg.o] Error 1
>>>make[1]: *** [drivers/scsi] Error 2
>>>make: *** [drivers] Error 2
>>>
>>>sg_jif_to_ms() is marked inline but used defore its function
>>>body is available. Moving it nearer the top of sg.c (together
>>>with sg_ms_to_jif() for consistency) fixes the problem.
>> 
>> 
>> While your patch is perfectly fine I think we can do better.
>> I think that we may try converting sg.c to use jiffies_to_msecs()
>> and msecs_to_jiffies() from <linux/time.h>.
>
>
>_Look_ at the patch.  It just moves code around for no reason.  Using 
>static inline prototypes should work just fine.  This patch isn't needed.

It's needed, and no it's not a compiler bug.

2.6.8-rc1-mm1 changed <linux/compiler-gcc3.h> to attach
__attribute__((always_inline)) to inline-declarations.
gcc-3.4, in turn, honors this literally: if it fails to
inline because the function body isn't visible at the
call site, then it generates a compiler error, exactly
as the kernel asked it to.

(And placing 'static inline' on a forward-declared function
is utterly pointless: if its body isn't visible it won't
be inlined, and if it is visible, the forward declaration
itself served no purpose.)

Now maybe Andrew shouldn't have merged that change, but
at least it shows us where inlining doesn't occur, allowing
us to fix the code or remove unnecessary inlines.

/Mikael

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Mikael Pettersson wrote:
> It's needed, and no it's not a compiler bug.

In fact, it is.  gcc isn't properly inlining functions where uses occur 
before implementation of the inlined function.

Or you could just call it "gcc is dumb" rather than a compiler bug.

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Adrian Bunk]

On Wed, Jul 14, 2004 at 02:36:04PM -0400, Jeff Garzik wrote:
> Mikael Pettersson wrote:
> >It's needed, and no it's not a compiler bug.
> 
> In fact, it is.  gcc isn't properly inlining functions where uses occur 
> before implementation of the inlined function.
> 
> Or you could just call it "gcc is dumb" rather than a compiler bug.

gcc 3.4 seems to be the first gcc version that could actually handle 
such cases.

But since the kernel uses -fno-unit-at-a-time, it doesn't work.

The problem with unit-at-a-time is that it might increase the stack 
usage. Arjan explained this in the "GCC 3.4 and broken inlining." thread 
that started Thursday.

> 	Jeff

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> Or you could just call it "gcc is dumb" rather than a compiler bug.

Yeah, but doing:

	static inline foo(void);

	bar()
	{
		...
		foo();
	}

	static inline foo(void)
	{
		...
	}

is pretty dumb too.  I don't see any harm if this compiler feature/problem
pushes us to fix the above in the obvious way.

Plus if we do, the inlining attempt actually succeed, on all compiler
versions.

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Andrew Morton wrote:
> Yeah, but doing:
> 
> 	static inline foo(void);
> 
> 	bar()
> 	{
> 		...
> 		foo();
> 	}
> 
> 	static inline foo(void)
> 	{
> 		...
> 	}
> 
> is pretty dumb too.  I don't see any harm if this compiler feature/problem
> pushes us to fix the above in the obvious way.


???  C does not require ordering of function _implementations_, except 
for this gcc brokenness.

The above example allows one to do what one normally does with 
non-inlines:  order code to enhance readability, and the compiler will 
Do The Right Thing and utilize it in the best way the CPU will function.

Just because you stick a modifier on a function doesn't mean it's time 
to stop using C as it was meant to be used :)

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> Andrew Morton wrote:
> > Yeah, but doing:
> > 
> > 	static inline foo(void);
> > 
> > 	bar()
> > 	{
> > 		...
> > 		foo();
> > 	}
> > 
> > 	static inline foo(void)
> > 	{
> > 		...
> > 	}
> > 
> > is pretty dumb too.  I don't see any harm if this compiler feature/problem
> > pushes us to fix the above in the obvious way.
> 
> 
> ???  C does not require ordering of function _implementations_, except 
> for this gcc brokenness.

Well.  Other compilers had that restriction iirc, and it's a pretty natural
thing to do.

> The above example allows one to do what one normally does with 
> non-inlines:  order code to enhance readability

I always expect to find those little helper functions earlier in the
compilation unit than their callsites.  Pascal-style.  Plus you don't have
the hassle of keeping the declaration and definition in sync.

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Matthew Wilcox]

On Wed, Jul 14, 2004 at 07:39:26PM -0400, Jeff Garzik wrote:
> ???  C does not require ordering of function _implementations_, except 
> for this gcc brokenness.

Actually, GCC has always required that inlines be specified first
for them to be inlined, even in earlier versions.  For example, from the
gcc 3.3 manual:

Some calls cannot be integrated for various reasons (in particular,
calls that precede the function's definition cannot be integrated, and
neither can recursive calls within the definition).  If there is a
nonintegrated call, then the function is compiled to assembler code as
usual.

> The above example allows one to do what one normally does with 
> non-inlines:  order code to enhance readability, and the compiler will 
> Do The Right Thing and utilize it in the best way the CPU will function.

I find it enhances readability to place functions in reverse order.
That way I know I can just go to the top of a file, and the first match
on the function's name is its definition.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jens Axboe]

On Wed, Jul 14 2004, Andrew Morton wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> > Or you could just call it "gcc is dumb" rather than a compiler bug.
> 
> Yeah, but doing:
> 
> 	static inline foo(void);
> 
> 	bar()
> 	{
> 		...
> 		foo();
> 	}
> 
> 	static inline foo(void)
> 	{
> 		...
> 	}
> 
> is pretty dumb too.  I don't see any harm if this compiler feature/problem
> pushes us to fix the above in the obvious way.

Excuse my ignorance, but why on earth would that be dumb? Looks
perfectly legit to me, and I have to agree with Jeff that the compiler
is exceedingly dumb if it fails to inline that case.

-- 
Jens Axboe

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[William Lee Irwin III]

Jeff Garzik <jgarzik@pobox.com> wrote:
>>> Or you could just call it "gcc is dumb" rather than a compiler bug.

On Wed, Jul 14 2004, Andrew Morton wrote:
[... code snippet ...]
>> is pretty dumb too.  I don't see any harm if this compiler feature/problem
>> pushes us to fix the above in the obvious way.

On Thu, Jul 15, 2004 at 07:56:56AM +0200, Jens Axboe wrote:
> Excuse my ignorance, but why on earth would that be dumb? Looks
> perfectly legit to me, and I have to agree with Jeff that the compiler
> is exceedingly dumb if it fails to inline that case.

Enter gcc...

Maybe "the obvious way" is sending a someone off to whip gcc into shape,
or possibly reporting it as a gcc problem.


-- wli

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Douglas Gilbert]

Andrew Morton wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>>Or you could just call it "gcc is dumb" rather than a compiler bug.
> 
> 
> Yeah, but doing:
> 
> 	static inline foo(void);
> 
> 	bar()
> 	{
> 		...
> 		foo();
> 	}
> 
> 	static inline foo(void)
> 	{
> 		...
> 	}
> 
> is pretty dumb too.  I don't see any harm if this compiler feature/problem
> pushes us to fix the above in the obvious way.

It might be dumb but C99 (ISO/IEC 9899:1999(E) ) says in
section 6.7.4 :
   "function-specifiers shall be used only in the
    declaration of an identifier for a function"

The only "function-specifier" defined in C99 is
"inline". So that seems to imply "inline" should
not appear in a function definition. If that is true
then you must declare an inline function before use
in order to pass the "inline" hint to the compiler.

Anyways C99 says very little about "inline" other than
some restrictions on its usage together with external
linkage. That seems to imply constructs that are legal
_without_ "inline" in front of a function declaration
should also be legal when "inline" is added (modulo
external linkage restrictions).

Doug Gilbert

[PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Mikael Pettersson]

gcc-3.4.1 errors out in 2.6.8-rc1-mm1 at drivers/scsi/sg.c:

drivers/scsi/sg.c: In function `sg_ioctl':
drivers/scsi/sg.c:209: sorry, unimplemented: inlining failed in call to 'sg_jif_to_ms': function body not available
drivers/scsi/sg.c:930: sorry, unimplemented: called from here
make[2]: *** [drivers/scsi/sg.o] Error 1
make[1]: *** [drivers/scsi] Error 2
make: *** [drivers] Error 2

sg_jif_to_ms() is marked inline but used defore its function
body is available. Moving it nearer the top of sg.c (together
with sg_ms_to_jif() for consistency) fixes the problem.

Signed-off-by: Mikael Pettersson <mikpe@csd.uu.se>

--- linux-2.6.8-rc1-mm1/drivers/scsi/sg.c.~1~	2004-07-14 12:58:34.000000000 +0200
+++ linux-2.6.8-rc1-mm1/drivers/scsi/sg.c	2004-07-14 13:45:26.000000000 +0200
@@ -225,6 +225,30 @@
 #define SZ_SG_REQ_INFO sizeof(sg_req_info_t)
 
 static int
+sg_ms_to_jif(unsigned int msecs)
+{
+	if ((UINT_MAX / 2U) < msecs)
+		return INT_MAX;	/* special case, set largest possible */
+	else
+		return ((int) msecs <
+			(INT_MAX / 1000)) ? (((int) msecs * HZ) / 1000)
+		    : (((int) msecs / 1000) * HZ);
+}
+
+static inline unsigned
+sg_jif_to_ms(int jifs)
+{
+	if (jifs <= 0)
+		return 0U;
+	else {
+		unsigned int j = (unsigned int) jifs;
+		return (j <
+			(UINT_MAX / 1000)) ? ((j * 1000) / HZ) : ((j / HZ) *
+								  1000);
+	}
+}
+
+static int
 sg_open(struct inode *inode, struct file *filp)
 {
 	int dev = iminor(inode);
@@ -2575,30 +2599,6 @@
 	free_pages((unsigned long) buff, order);
 }
 
-static int
-sg_ms_to_jif(unsigned int msecs)
-{
-	if ((UINT_MAX / 2U) < msecs)
-		return INT_MAX;	/* special case, set largest possible */
-	else
-		return ((int) msecs <
-			(INT_MAX / 1000)) ? (((int) msecs * HZ) / 1000)
-		    : (((int) msecs / 1000) * HZ);
-}
-
-static inline unsigned
-sg_jif_to_ms(int jifs)
-{
-	if (jifs <= 0)
-		return 0U;
-	else {
-		unsigned int j = (unsigned int) jifs;
-		return (j <
-			(UINT_MAX / 1000)) ? ((j * 1000) / HZ) : ((j / HZ) *
-								  1000);
-	}
-}
-
 static unsigned char allow_ops[] = { TEST_UNIT_READY, REQUEST_SENSE,
 	INQUIRY, READ_CAPACITY, READ_BUFFER, READ_6, READ_10, READ_12,
 	MODE_SENSE, MODE_SENSE_10, LOG_SENSE

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Mikael Pettersson wrote:
> gcc-3.4.1 errors out in 2.6.8-rc1-mm1 at drivers/scsi/sg.c:
> 
> drivers/scsi/sg.c: In function `sg_ioctl':
> drivers/scsi/sg.c:209: sorry, unimplemented: inlining failed in call to 'sg_jif_to_ms': function body not available
> drivers/scsi/sg.c:930: sorry, unimplemented: called from here
> make[2]: *** [drivers/scsi/sg.o] Error 1
> make[1]: *** [drivers/scsi] Error 2
> make: *** [drivers] Error 2
> 
> sg_jif_to_ms() is marked inline but used defore its function
> body is available. Moving it nearer the top of sg.c (together
> with sg_ms_to_jif() for consistency) fixes the problem.
> 
> Signed-off-by: Mikael Pettersson <mikpe@csd.uu.se>


This looks like a compiler bug to me.

sg_jif_to_ms is properly declared at the top of the file...

 > static int sg_ms_to_jif(unsigned int msecs);
 > static inline unsigned sg_jif_to_ms(int jifs);
 > static int sg_allow_access(unsigned char opcode, char dev_type);

If gcc is insisting that prototypes for inlines no longer work, we have 
a lot of code churn on our hands ;-(  Grumble.

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Adrian Bunk]

On Wed, Jul 14, 2004 at 11:52:46AM -0400, Jeff Garzik wrote:
>...
> If gcc is insisting that prototypes for inlines no longer work, we have 
> a lot of code churn on our hands ;-(  Grumble.

I've counted at about 30 files with such problems in a full i386 
2.6.7-mm7 compile.

I've already sent patches for some of them (e.g. the dmascc.c one), and 
they are usually pretty straightforward.

> 	Jeff

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Adrian Bunk wrote:
> On Wed, Jul 14, 2004 at 11:52:46AM -0400, Jeff Garzik wrote:
> 
>>...
>>If gcc is insisting that prototypes for inlines no longer work, we have 
>>a lot of code churn on our hands ;-(  Grumble.
> 
> 
> I've counted at about 30 files with such problems in a full i386 
> 2.6.7-mm7 compile.
> 
> I've already sent patches for some of them (e.g. the dmascc.c one), and 
> they are usually pretty straightforward.

This is not a problem with the kernel.

All these files have been functioning just fine for years, with properly 
prototyped static inline functions.

Though there is a the claim that '#define inline always_inline' is 
leading to all this breakage.

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Adrian Bunk]

On Wed, Jul 14, 2004 at 12:44:44PM -0400, Jeff Garzik wrote:
> Adrian Bunk wrote:
> >On Wed, Jul 14, 2004 at 11:52:46AM -0400, Jeff Garzik wrote:
> >
> >>...
> >>If gcc is insisting that prototypes for inlines no longer work, we have 
> >>a lot of code churn on our hands ;-(  Grumble.
> >
> >
> >I've counted at about 30 files with such problems in a full i386 
> >2.6.7-mm7 compile.
> >
> >I've already sent patches for some of them (e.g. the dmascc.c one), and 
> >they are usually pretty straightforward.
> 
> This is not a problem with the kernel.
> 
> All these files have been functioning just fine for years, with properly 
> prototyped static inline functions.

Add -Winline to the compile flags, and name one gcc version that is able 
to inline them all in sg.c ...

> Though there is a the claim that '#define inline always_inline' is 
> leading to all this breakage.

gcc 3.4 is just complaining louder that it can't inline something it was 
told to inline.

> 	Jeff

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Bartlomiej Zolnierkiewicz]

FYI 'inlining fix' was just merged as part of viro's sparse cleanups

I still would like somebody to comment on idea of converting sg.c
to use standard inlines from <linux/time.h> ...

On Wednesday 14 of July 2004 18:54, Adrian Bunk wrote:
> On Wed, Jul 14, 2004 at 12:44:44PM -0400, Jeff Garzik wrote:
> > Adrian Bunk wrote:
> > >On Wed, Jul 14, 2004 at 11:52:46AM -0400, Jeff Garzik wrote:
> > >>...
> > >>If gcc is insisting that prototypes for inlines no longer work, we have
> > >>a lot of code churn on our hands ;-(  Grumble.
> > >
> > >I've counted at about 30 files with such problems in a full i386
> > >2.6.7-mm7 compile.
> > >
> > >I've already sent patches for some of them (e.g. the dmascc.c one), and
> > >they are usually pretty straightforward.
> >
> > This is not a problem with the kernel.
> >
> > All these files have been functioning just fine for years, with properly
> > prototyped static inline functions.
>
> Add -Winline to the compile flags, and name one gcc version that is able
> to inline them all in sg.c ...
>
> > Though there is a the claim that '#define inline always_inline' is
> > leading to all this breakage.
>
> gcc 3.4 is just complaining louder that it can't inline something it was
> told to inline.
>
> > 	Jeff
>
> cu
> Adrian

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Adrian Bunk]

On Wed, Jul 14, 2004 at 07:31:12PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> FYI 'inlining fix' was just merged as part of viro's sparse cleanups
> 
> I still would like somebody to comment on idea of converting sg.c
> to use standard inlines from <linux/time.h> ...

I've already discussed this with Doug a few days ago, and he agreed.

Below is my current patch against 2.6.8-rc1-mm1.

I'm not yet 100% sure whether it's correct, so please double-check it.



[patch] kill local sg_ms_to_jif/sg_jif_to_ms functions and use
        msecs_to_jiffies/jiffies_to_msecs instead


Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

--- linux-2.6.8-rc1-mm1-full-3.4/drivers/scsi/sg.c.old	2004-07-14 20:19:09.000000000 +0200
+++ linux-2.6.8-rc1-mm1-full-3.4/drivers/scsi/sg.c	2004-07-14 20:21:50.000000000 +0200
@@ -205,8 +205,6 @@
 static Sg_request *sg_add_request(Sg_fd * sfp);
 static int sg_remove_request(Sg_fd * sfp, Sg_request * srp);
 static int sg_res_in_use(Sg_fd * sfp);
-static int sg_ms_to_jif(unsigned int msecs);
-static inline unsigned sg_jif_to_ms(int jifs);
 static int sg_allow_access(unsigned char opcode, char dev_type);
 static int sg_build_direct(Sg_request * srp, Sg_fd * sfp, int dxfer_len);
 static Sg_device *sg_get_dev(int dev);
@@ -612,7 +610,7 @@
 			return -EBUSY;	/* reserve buffer already being used */
 		}
 	}
-	timeout = sg_ms_to_jif(srp->header.timeout);
+	timeout = msecs_to_jiffies(srp->header.timeout);
 	if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof (cmnd))) {
 		sg_remove_request(sfp, srp);
 		return -EMSGSIZE;
@@ -929,7 +927,7 @@
 					    srp->header.driver_status;
 					rinfo[val].duration =
 					    srp->done ? srp->header.duration :
-					    sg_jif_to_ms(
+					    jiffies_to_msecs(
 						jiffies - srp->header.duration);
 					rinfo[val].orphan = srp->orphan;
 					rinfo[val].sg_io_owned = srp->sg_io_owned;
@@ -1251,7 +1249,7 @@
 	srp->header.resid = SCpnt->resid;
 	/* N.B. unit of duration changes here from jiffies to millisecs */
 	srp->header.duration =
-	    sg_jif_to_ms(jiffies - (int) srp->header.duration);
+	    jiffies_to_msecs(jiffies - srp->header.duration);
 	if (0 != SRpnt->sr_result) {
 		memcpy(srp->sense_b, SRpnt->sr_sense_buffer,
 		       sizeof (srp->sense_b));
@@ -2575,30 +2573,6 @@
 	free_pages((unsigned long) buff, order);
 }
 
-static int
-sg_ms_to_jif(unsigned int msecs)
-{
-	if ((UINT_MAX / 2U) < msecs)
-		return INT_MAX;	/* special case, set largest possible */
-	else
-		return ((int) msecs <
-			(INT_MAX / 1000)) ? (((int) msecs * HZ) / 1000)
-		    : (((int) msecs / 1000) * HZ);
-}
-
-static inline unsigned
-sg_jif_to_ms(int jifs)
-{
-	if (jifs <= 0)
-		return 0U;
-	else {
-		unsigned int j = (unsigned int) jifs;
-		return (j <
-			(UINT_MAX / 1000)) ? ((j * 1000) / HZ) : ((j / HZ) *
-								  1000);
-	}
-}
-
 static unsigned char allow_ops[] = { TEST_UNIT_READY, REQUEST_SENSE,
 	INQUIRY, READ_CAPACITY, READ_BUFFER, READ_6, READ_10, READ_12,
 	MODE_SENSE, MODE_SENSE_10, LOG_SENSE
@@ -2961,7 +2935,7 @@
 	for (k = 0; (fp = sg_get_nth_sfp(sdp, k)); ++k) {
 		seq_printf(s, "   FD(%d): timeout=%dms bufflen=%d "
 			   "(res)sgat=%d low_dma=%d\n", k + 1,
-			   sg_jif_to_ms(fp->timeout),
+			   jiffies_to_msecs(fp->timeout),
 			   fp->reserve.bufflen,
 			   (int) fp->reserve.k_use_sg,
 			   (int) fp->low_dma);
@@ -2997,8 +2971,8 @@
 				seq_printf(s, " dur=%d", hp->duration);
 			else
 				seq_printf(s, " t_o/elap=%d/%d",
-				  new_interface ? hp->timeout : sg_jif_to_ms(fp->timeout),
-				  sg_jif_to_ms(hp->duration ? (jiffies - hp->duration) : 0));
+				  new_interface ? hp->timeout : jiffies_to_msecs(fp->timeout),
+				  jiffies_to_msecs(hp->duration ? (jiffies - hp->duration) : 0));
 			seq_printf(s, "ms sgat=%d op=0x%02x\n", usg,
 				   (int) srp->data.cmd_opcode);
 		}

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Your patch looks OK to me...

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Douglas Gilbert]

Adrian Bunk wrote:
> On Wed, Jul 14, 2004 at 07:31:12PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
>>FYI 'inlining fix' was just merged as part of viro's sparse cleanups
>>
>>I still would like somebody to comment on idea of converting sg.c
>>to use standard inlines from <linux/time.h> ...
> 
> 
> I've already discussed this with Doug a few days ago, and he agreed.
> 
> Below is my current patch against 2.6.8-rc1-mm1.
> 
> I'm not yet 100% sure whether it's correct, so please double-check it.

Adrian,
This patch looks fine.

Doug Gilbert

> 
> [patch] kill local sg_ms_to_jif/sg_jif_to_ms functions and use
>         msecs_to_jiffies/jiffies_to_msecs instead
> 
> 
> Signed-off-by: Adrian Bunk <bunk@fs.tum.de>
> 
> --- linux-2.6.8-rc1-mm1-full-3.4/drivers/scsi/sg.c.old	2004-07-14 20:19:09.000000000 +0200
> +++ linux-2.6.8-rc1-mm1-full-3.4/drivers/scsi/sg.c	2004-07-14 20:21:50.000000000 +0200
> @@ -205,8 +205,6 @@
>  static Sg_request *sg_add_request(Sg_fd * sfp);
>  static int sg_remove_request(Sg_fd * sfp, Sg_request * srp);
>  static int sg_res_in_use(Sg_fd * sfp);
> -static int sg_ms_to_jif(unsigned int msecs);
> -static inline unsigned sg_jif_to_ms(int jifs);
>  static int sg_allow_access(unsigned char opcode, char dev_type);
>  static int sg_build_direct(Sg_request * srp, Sg_fd * sfp, int dxfer_len);
>  static Sg_device *sg_get_dev(int dev);
> @@ -612,7 +610,7 @@
>  			return -EBUSY;	/* reserve buffer already being used */
>  		}
>  	}
> -	timeout = sg_ms_to_jif(srp->header.timeout);
> +	timeout = msecs_to_jiffies(srp->header.timeout);
>  	if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof (cmnd))) {
>  		sg_remove_request(sfp, srp);
>  		return -EMSGSIZE;
> @@ -929,7 +927,7 @@
>  					    srp->header.driver_status;
>  					rinfo[val].duration =
>  					    srp->done ? srp->header.duration :
> -					    sg_jif_to_ms(
> +					    jiffies_to_msecs(
>  						jiffies - srp->header.duration);
>  					rinfo[val].orphan = srp->orphan;
>  					rinfo[val].sg_io_owned = srp->sg_io_owned;
> @@ -1251,7 +1249,7 @@
>  	srp->header.resid = SCpnt->resid;
>  	/* N.B. unit of duration changes here from jiffies to millisecs */
>  	srp->header.duration =
> -	    sg_jif_to_ms(jiffies - (int) srp->header.duration);
> +	    jiffies_to_msecs(jiffies - srp->header.duration);
>  	if (0 != SRpnt->sr_result) {
>  		memcpy(srp->sense_b, SRpnt->sr_sense_buffer,
>  		       sizeof (srp->sense_b));
> @@ -2575,30 +2573,6 @@
>  	free_pages((unsigned long) buff, order);
>  }
>  
> -static int
> -sg_ms_to_jif(unsigned int msecs)
> -{
> -	if ((UINT_MAX / 2U) < msecs)
> -		return INT_MAX;	/* special case, set largest possible */
> -	else
> -		return ((int) msecs <
> -			(INT_MAX / 1000)) ? (((int) msecs * HZ) / 1000)
> -		    : (((int) msecs / 1000) * HZ);
> -}
> -
> -static inline unsigned
> -sg_jif_to_ms(int jifs)
> -{
> -	if (jifs <= 0)
> -		return 0U;
> -	else {
> -		unsigned int j = (unsigned int) jifs;
> -		return (j <
> -			(UINT_MAX / 1000)) ? ((j * 1000) / HZ) : ((j / HZ) *
> -								  1000);
> -	}
> -}
> -
>  static unsigned char allow_ops[] = { TEST_UNIT_READY, REQUEST_SENSE,
>  	INQUIRY, READ_CAPACITY, READ_BUFFER, READ_6, READ_10, READ_12,
>  	MODE_SENSE, MODE_SENSE_10, LOG_SENSE
> @@ -2961,7 +2935,7 @@
>  	for (k = 0; (fp = sg_get_nth_sfp(sdp, k)); ++k) {
>  		seq_printf(s, "   FD(%d): timeout=%dms bufflen=%d "
>  			   "(res)sgat=%d low_dma=%d\n", k + 1,
> -			   sg_jif_to_ms(fp->timeout),
> +			   jiffies_to_msecs(fp->timeout),
>  			   fp->reserve.bufflen,
>  			   (int) fp->reserve.k_use_sg,
>  			   (int) fp->low_dma);
> @@ -2997,8 +2971,8 @@
>  				seq_printf(s, " dur=%d", hp->duration);
>  			else
>  				seq_printf(s, " t_o/elap=%d/%d",
> -				  new_interface ? hp->timeout : sg_jif_to_ms(fp->timeout),
> -				  sg_jif_to_ms(hp->duration ? (jiffies - hp->duration) : 0));
> +				  new_interface ? hp->timeout : jiffies_to_msecs(fp->timeout),
> +				  jiffies_to_msecs(hp->duration ? (jiffies - hp->duration) : 0));
>  			seq_printf(s, "ms sgat=%d op=0x%02x\n", usg,
>  				   (int) srp->data.cmd_opcode);
>  		}
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Andrew Morton]

Adrian Bunk <bunk@fs.tum.de> wrote:
>
> I'm not yet 100% sure whether it's correct, so please double-check it.
> 
> 
> 
>  [patch] kill local sg_ms_to_jif/sg_jif_to_ms functions and use
>          msecs_to_jiffies/jiffies_to_msecs instead

Looks good here - thanks.

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> I still would like somebody to comment on idea of converting sg.c
> to use standard inlines from <linux/time.h> ...


I agree this should be done...

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Also, it would probably be better for some of these changes to simply be 
made non-inline.

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 14 of July 2004 14:16, Mikael Pettersson wrote:
> gcc-3.4.1 errors out in 2.6.8-rc1-mm1 at drivers/scsi/sg.c:
>
> drivers/scsi/sg.c: In function `sg_ioctl':
> drivers/scsi/sg.c:209: sorry, unimplemented: inlining failed in call to
> 'sg_jif_to_ms': function body not available drivers/scsi/sg.c:930: sorry,
> unimplemented: called from here
> make[2]: *** [drivers/scsi/sg.o] Error 1
> make[1]: *** [drivers/scsi] Error 2
> make: *** [drivers] Error 2
>
> sg_jif_to_ms() is marked inline but used defore its function
> body is available. Moving it nearer the top of sg.c (together
> with sg_ms_to_jif() for consistency) fixes the problem.

While your patch is perfectly fine I think we can do better.
I think that we may try converting sg.c to use jiffies_to_msecs()
and msecs_to_jiffies() from <linux/time.h>.

> Signed-off-by: Mikael Pettersson <mikpe@csd.uu.se>
>
> --- linux-2.6.8-rc1-mm1/drivers/scsi/sg.c.~1~	2004-07-14 12:58:34.000000000
> +0200 +++ linux-2.6.8-rc1-mm1/drivers/scsi/sg.c	2004-07-14
> 13:45:26.000000000 +0200 @@ -225,6 +225,30 @@
>  #define SZ_SG_REQ_INFO sizeof(sg_req_info_t)
>
>  static int
> +sg_ms_to_jif(unsigned int msecs)
> +{
> +	if ((UINT_MAX / 2U) < msecs)
> +		return INT_MAX;	/* special case, set largest possible */

I don't understand what for is this special case.

> +	else
> +		return ((int) msecs <
> +			(INT_MAX / 1000)) ? (((int) msecs * HZ) / 1000)
> +		    : (((int) msecs / 1000) * HZ);
> +}

jiffies are unsigned long, isn't this buggy on 64-bit archs?

> +static inline unsigned
> +sg_jif_to_ms(int jifs)
> +{

ditto

> +	if (jifs <= 0)
> +		return 0U;
> +	else {
> +		unsigned int j = (unsigned int) jifs;
> +		return (j <
> +			(UINT_MAX / 1000)) ? ((j * 1000) / HZ) : ((j / HZ) *
> +								  1000);
> +	}
> +}

also rounding up is missing (jiffies_to_msecs()/msecs_to_jiffies() does it)

Bartlomiej

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> On Wednesday 14 of July 2004 14:16, Mikael Pettersson wrote:
> 
>>gcc-3.4.1 errors out in 2.6.8-rc1-mm1 at drivers/scsi/sg.c:
>>
>>drivers/scsi/sg.c: In function `sg_ioctl':
>>drivers/scsi/sg.c:209: sorry, unimplemented: inlining failed in call to
>>'sg_jif_to_ms': function body not available drivers/scsi/sg.c:930: sorry,
>>unimplemented: called from here
>>make[2]: *** [drivers/scsi/sg.o] Error 1
>>make[1]: *** [drivers/scsi] Error 2
>>make: *** [drivers] Error 2
>>
>>sg_jif_to_ms() is marked inline but used defore its function
>>body is available. Moving it nearer the top of sg.c (together
>>with sg_ms_to_jif() for consistency) fixes the problem.
> 
> 
> While your patch is perfectly fine I think we can do better.
> I think that we may try converting sg.c to use jiffies_to_msecs()
> and msecs_to_jiffies() from <linux/time.h>.


_Look_ at the patch.  It just moves code around for no reason.  Using 
static inline prototypes should work just fine.  This patch isn't needed.

	Jeff

Re: [PATCH][2.6.8-rc1-mm1] drivers/scsi/sg.c gcc341 inlining fix

[Andrew Morton]

Mikael Pettersson <mikpe@csd.uu.se> wrote:
>
> gcc-3.4.1 errors out in 2.6.8-rc1-mm1 at drivers/scsi/sg.c:
> 
> drivers/scsi/sg.c: In function `sg_ioctl':
> drivers/scsi/sg.c:209: sorry, unimplemented: inlining failed in call to 'sg_jif_to_ms': function body not available
> drivers/scsi/sg.c:930: sorry, unimplemented: called from here
> make[2]: *** [drivers/scsi/sg.o] Error 1
> make[1]: *** [drivers/scsi] Error 2
> make: *** [drivers] Error 2
> 
> sg_jif_to_ms() is marked inline but used defore its function
> body is available. Moving it nearer the top of sg.c (together
> with sg_ms_to_jif() for consistency) fixes the problem.
> 
>...
>  static int
> +sg_ms_to_jif(unsigned int msecs)
> +{
> +	if ((UINT_MAX / 2U) < msecs)
> +		return INT_MAX;	/* special case, set largest possible */
> +	else
> +		return ((int) msecs <
> +			(INT_MAX / 1000)) ? (((int) msecs * HZ) / 1000)
> +		    : (((int) msecs / 1000) * HZ);
> +}
> +
> +static inline unsigned
> +sg_jif_to_ms(int jifs)
> +{
> +	if (jifs <= 0)
> +		return 0U;
> +	else {
> +		unsigned int j = (unsigned int) jifs;
> +		return (j <
> +			(UINT_MAX / 1000)) ? ((j * 1000) / HZ) : ((j / HZ) *
> +								  1000);
> +	}
> +}
> +


We have standard jiffies_to_msecs() and msecs_to_jiffies() functions in
include/linux/time.h.  Can we please make these sg-private versions go away
altogether?