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: 4071 bytes --]

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.

> 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.

Fwiw, I'm pretty sure the kernel has relied on zero-initialization of
non-static globals for many years.

> If you're strongly against it, fine, C11 says it's zero... but it makes
> me a bit nervous nevertheless.
> 

-- 
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 --]