Re: [PATCH v3 10/13] flow: Move flow_count from context structure to a global

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8166 bytes --]

On Wed, Jan 03, 2024 at 08:08:34AM +0100, Stefano Brivio wrote:
> On Wed, 3 Jan 2024 14:54:27 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > I'm not sure where to get the actual text of the standards
> 
> Let me answer this first: one (the?) trick is to use so-called final
> drafts, which are made freely available (same as working drafts) by the
> Working Group.
> 
> Those are not the same as the standards, but differences from the final
> draft are also published... and they are usually not substantial.
> 
> This is _very_ informative:
>   https://stackoverflow.com/questions/81656/where-do-i-find-the-current-c-or-c-standard-documents

Ah, thanks.

> Wikipedia also has the links, by the way. Anyway, in practice:
> 
> - C11: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
> - C99: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> - C89:
>   https://web.archive.org/web/20200909074736if_/https://www.pdf-archive.com/2014/10/02/ansi-iso-9899-1990-1/ansi-iso-9899-1990-1.pdf
> 
> > On Tue, Jan 02, 2024 at 07:13:35PM +0100, Stefano Brivio wrote:
> > > On Sun, 31 Dec 2023 16:58:39 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Dec 28, 2023 at 07:25:18PM +0100, Stefano Brivio wrote:  
> > > > > On Thu, 21 Dec 2023 17:15:46 +1100
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >     
> > > > > > In general, the passt code is a bit haphazard about what's a true global
> > > > > > variable and what's in the quasi-global 'context structure'.  The
> > > > > > flow_count field is one such example: it's in the context structure,
> > > > > > although it's really part of the same data structure as flowtab[], which
> > > > > > is a genuine global.    
> > > > > 
> > > > > Well, the reason is that flow_tab[FLOW_MAX] might be problematically
> > > > > too big to live on the stack, unlike flow_count.
> > > > > 
> > > > > But anyway, as far as thoughts of multithreading are concerned, both
> > > > > should probably be global. And sure, it's more consistent this way.
> > > > >     
> > > > > > Move flow_count to be a regular global to match.  For now it needs to be
> > > > > > public, rather than static, but we expect to be able to change that in
> > > > > > future.    
> > > > > 
> > > > > If it's not static, it should be initialised, and that's not done here.    
> > > > 
> > > > Uh... what?  "static" here is meaning module-global rather than
> > > > global-global, which has no bearing on initialisation.  AFAIK globals
> > > > are zero-initialised whether they're static or not.  
> > > 
> > > ...and to my utter surprise, I just discovered that if you talk C11,
> > > you're right. From the N1570 draft (ISO/IEC 9899:201x), Section 6.7.9
> > > "Initialization", clause 10:
> > > 
> > >   If an object that has automatic storage duration is not initialized
> > >   explicitly, its value is indeterminate. If an object that has static
> > >   or thread storage duration is not initialized explicitly, then:
> > > 
> > >   [...]
> > > 
> > >   — if it has arithmetic type, it is initialized to (positive or
> > >     unsigned) zero;
> > > 
> > > And 'flow_count' has thread storage duration.  
> > 
> > No.. I don't think it does.  AFAICT only thread-local variables have
> > thread storage duration.  As a global flow_count will have static
> > storage duration, even without the static keyword.
> 
> So, C11 defines static storage duration here:
> 
>   6.2.4 Storage durations of objects
> 
>   [...]
> 
>   3 An object whose identifier is declared without the storage-class
>   specifier _Thread_local, and either with external or internal linkage
>   or with the storage-class specifier static, has static storage
>   duration. Its lifetime is the entire execution of the program and its
>   stored value is initialized only once, prior to program startup.
> 
> do we have any linkage here? I would have said no -- but, going back
> to C99 for this, "6.2.2 Linkages of identifiers":
> 
>   5 [...] If the declaration of an identifier for an object has file
>   scope and no storage-class specifier, its linkage is external.
> 
> which supports your paragraph below.

Right.

> By the way, C11 now says:
> 
>   6.11.2 Linkages of identifiers
> 
>   1 Declaring an identifier with internal linkage at file scope without
>   the static storage-class specifier is an obsolescent feature

Ok.  I'm not even sure how you would do that.

> > > In C99, however (draft
> > > N1256), Section 6.7.8 "Initialization", clause 10:
> > > 
> > >   If an object that has automatic storage duration is not initialized
> > >   explicitly, its value is indeterminate. If an object that has static
> > >   storage duration is not initialized explicitly, then:
> > > 
> > >   [...]
> > > 
> > > note the missing "or thread storage duration".
> > > 
> > > C89, the one I was actually basing my observation on, says, at 3.5.7
> > > "Initialization":
> > > 
> > >   If an object that has static storage duration is not initialized
> > >   explicitly, it is initialized implicitly as if every member that has
> > >   arithmetic type were assigned 0 and every member that has pointer type
> > >   were assigned a null pointer constant.  If an object that has
> > >   automatic storage duration is not initialized explicitly, its value is
> > >   indeterminate.
> > > 
> > > so... um. We won't go back to C99. But to me, and maybe others, not
> > > having a "= 0;" for a "global" means pretty much that we don't rely on
> > > any particular initial value.  
> > 
> > Again, I'm pretty sure that's not true, even for C99 and C89.  AIUI,
> > 'static' locals and *all* globals have "static storage diration".
> > 
> > I'm not sure where to get the actual text of the standards but see for
> > example
> > 
> > https://en.cppreference.com/w/c/language/static_storage_duration
> > 
> > Here 'flow_count' has external linkage, thus satisfying the conditions
> > for static storage duration.
> 
> Right. Well, for C99 and C11 at least. For C89 things are slightly
> different:
> 
>   6.1.2.4 Storage durations of objects
> 
>   [...]
> 
>   An object whose identifier is declared with external or internal
>   linkage. or with the storage-class specifier static has static storage
>   duration.
> 
>   [...]
> 
>   An object whose identifier is declared with no linkage and without the
>   storage-class specifier static has automatic storage duration.
> 
> You might say it has external linkage. But it was not *declared with*
> external linkage -- it just happens to have it (C89 and C99 don't
> differ here).

Hrm.  We do have:
	extern unsigned flow_first_free;
in flow_table.h.  Does that cound as declaring with external linkage?

> > Fwiw, I'm pretty sure the kernel has relied on zero-initialization of
> > non-static globals for many years.
> 
> True, and the opposite is even considered as a style issue since 2007,
> commit f0a594c1c74f ("update checkpatch.pl to version 0.08"). I also
> found a discussion similar to this one:
>   https://lore.kernel.org/all/20201102184147.GA42288@localhost/#r
> 
> Anyway... a couple of years before that, it must have been a gcc version
> in the late 2.x, I actually hit an issue with it. Was it a compiler
> issue, or the correct interpretation of C89? Or maybe something on the
> lines of:
>   https://www.thegoodpenguin.co.uk/blog/u-boot-relocation-bss-hang/

If it was an embedded setup, that last one is certainly possible.
Zeroing the BSS is typically the loader's job, and I've certainly seen
loader implementations - particularly in embedded firmware - that got
this wrong.

> ? I'll never know/remember. And then, after reading the standard, I
> started obsessively adding those = 0. Well, good to know I can stop
> doing it now. :)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]