Re: [PATCH v2 11/22] flow: Clarify flow entry life cycle, introduce uniform logging

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

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

On Sun, Feb 18, 2024 at 10:00:04PM +0100, Stefano Brivio wrote:
> On Tue,  6 Feb 2024 12:17:23 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Our allocation scheme for flow entries means there are some non-obvious
> > constraints on when what things can be done with an entry.  Add a big doc
> > comment explaining the life cycle.
> > 
> > In addition, make a FLOW_START() macro to mark one of the important
> > transitions.  This encourages correct usage, by making it natural to only
> > access the flow type specific structure after calling it.  It also logs
> > that a new flow has been created, which is useful for debugging.
> > 
> > We also add logging when a flow's lifecycle ends.  This doesn't need a new
> > helper, because it can only happen either from flow_alloc_cancel() or from
> > the flow deferred handler.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c       | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  flow.h       |  5 ++++
> >  tcp.c        | 15 +++++------
> >  tcp_splice.c | 11 ++++----
> >  tcp_splice.h |  5 ++--
> >  5 files changed, 94 insertions(+), 18 deletions(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index beb9749c..a155b54b 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -34,6 +34,45 @@ static_assert(ARRAY_SIZE(flow_proto) == FLOW_NUM_TYPES,
> >  
> >  /* Global Flow Table */
> >  
> > +/**
> > + * DOC: Theory of Operation - flow entry life cycle
> > + *
> > + * An individual flow table entry moves through these logical states, usually in
> > + * this order.
> > + *
> > + *    FREE - Part of the general pool of free flow table entries
> > + *        Operations:
> > + *            - flow_alloc() finds an entry and moves it to ALLOC state
> > + *
> > + *    ALLOC - A tentatively allocated entry
> > + *        Operations:
> > + *            - flow_alloc_cancel() returns the entry to FREE state
> > + *            - FLOW_START() set the entry's type and moves to START state
> > + *        Caveats:
> > + *            - It's not safe to write fields in the flow entry
> > + *            - It's not safe to allocate other entries with flow_alloc()
> 
> I'm not entirely sure what you mean here -- is this in the sense of
> s/other/further/ ?

Yes.  I've changed the word to "further", which I agree is clearer.

> > + *            - It's not safe to return to the main epoll loop
> 
> ..."before FLOW_START() is called on the entry"?

Right, because FLOW_START() moves to START state, where returning to
the epoll loop *is* allowed.  I've added a note which I hope makes
that clearer.

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