From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v3 10/13] flow: Move flow_count from context structure to a global
Date: Sun, 7 Jan 2024 16:23:42 +1100 [thread overview]
Message-ID: <ZZo1Xu5gWJo5uuO_@zatzit> (raw)
In-Reply-To: <20240105085513.5eadf3fc@elisabeth>
[-- Attachment #1: Type: text/plain, Size: 9771 bytes --]
On Fri, Jan 05, 2024 at 08:55:13AM +0100, Stefano Brivio wrote:
> On Thu, 4 Jan 2024 20:51:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > 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.
>
> By doing what I *thought* you were doing (see below): "int x" at file
> scope (outside functions), no static, no extern declaration, nothing.
Oh, ok. Even without that, I think the behaviour has to be the same.
Plain "int x" at file scope is a public variable, and a linked module
could "extern" it, even if that extern isn't visible while we're
compiling this module.
> > > > > 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?
>
> Gosh, sorry... yes, it also counts as me completely missing it.
>
> > > > 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.
>
> Embedded setup, yes, but in a Linux kernel (early 2.4.x). No 'extern'
> there, though.
Right. Depending on platform, the kernel sometimes has a loader for a
later stage so does need to clear BSS. Or it might need to as a
workaround for a broken firmware loader. I'm pretty sure I've also
seem Linux bugs on some embedded platforms that involved failing to
clear the BSS.
--
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 --]
next prev parent reply other threads:[~2024-01-07 5:23 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-21 6:15 [PATCH v3 00/13] Manage more flow related things from generic flow code David Gibson
2023-12-21 6:15 ` [PATCH v3 01/13] flow: Make flow_table.h #include the protocol specific headers it needs David Gibson
2023-12-21 6:15 ` [PATCH v3 02/13] treewide: Standardise on 'now' for current timestamp variables David Gibson
2023-12-21 6:15 ` [PATCH v3 03/13] tcp, tcp_splice: Remove redundant handling from tcp_timer() David Gibson
2023-12-21 6:15 ` [PATCH v3 04/13] tcp, tcp_splice: Move per-type cleanup logic into per-type helpers David Gibson
2023-12-21 6:15 ` [PATCH v3 05/13] flow, tcp: Add flow-centric dispatch for deferred flow handling David Gibson
2023-12-28 18:24 ` Stefano Brivio
2023-12-31 5:56 ` David Gibson
2024-01-02 18:13 ` Stefano Brivio
2024-01-03 3:45 ` David Gibson
2023-12-21 6:15 ` [PATCH v3 06/13] flow, tcp: Add handling for per-flow timers David Gibson
2023-12-21 6:15 ` [PATCH v3 07/13] epoll: Better handling of number of epoll types David Gibson
2023-12-21 6:15 ` [PATCH v3 08/13] tcp, tcp_splice: Avoid double layered dispatch for connected TCP sockets David Gibson
2023-12-21 6:15 ` [PATCH v3 09/13] flow: Move flow_log_() to near top of flow.c David Gibson
2023-12-21 6:15 ` [PATCH v3 10/13] flow: Move flow_count from context structure to a global David Gibson
2023-12-28 18:25 ` Stefano Brivio
2023-12-31 5:58 ` David Gibson
2024-01-02 18:13 ` Stefano Brivio
2024-01-03 3:54 ` David Gibson
2024-01-03 7:08 ` Stefano Brivio
2024-01-04 9:51 ` David Gibson
2024-01-05 7:55 ` Stefano Brivio
2024-01-07 5:23 ` David Gibson [this message]
2023-12-21 6:15 ` [PATCH v3 11/13] flow: Abstract allocation of new flows with helper function David Gibson
2023-12-21 6:15 ` [PATCH v3 12/13] flow: Enforce that freeing of closed flows must happen in deferred handlers David Gibson
2023-12-21 6:15 ` [PATCH v3 13/13] flow: Avoid moving flow entries to compact table David Gibson
2023-12-28 18:25 ` Stefano Brivio
2023-12-30 10:33 ` Stefano Brivio
2024-01-01 12:01 ` David Gibson
2024-01-02 18:13 ` Stefano Brivio
2024-01-04 10:02 ` David Gibson
2024-01-05 8:33 ` Stefano Brivio
2024-01-05 9:39 ` David Gibson
2024-01-05 10:27 ` Stefano Brivio
2024-01-06 11:32 ` David Gibson
2024-01-06 13:02 ` Stefano Brivio
2024-01-07 5:20 ` David Gibson
2024-01-01 10:44 ` David Gibson
2024-01-02 18:13 ` Stefano Brivio
2024-01-05 9:45 ` David Gibson
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=ZZo1Xu5gWJo5uuO_@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/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.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
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 IMAP folder(s).