Re: [PATCH 3/5] tcp, flow: Replace per-connection in_epoll flag with epollfd in flow_common

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

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

On Tue, Oct 07, 2025 at 02:15:03PM +0200, Stefano Brivio wrote:
> On Tue, 7 Oct 2025 14:07:39 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > On 07/10/2025 11:51, Stefano Brivio wrote:
> > > On Tue, 7 Oct 2025 17:07:37 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > >> On Fri, Oct 03, 2025 at 05:27:15PM +0200, Laurent Vivier wrote:  
> > >>> The in_epoll boolean flag in tcp_tap_conn and tcp_splice_conn only tracked
> > >>> whether a connection was registered with epoll, not which epoll instance.
> > >>> This limited flexibility for future multi-epoll support.
> > >>>
> > >>> Replace the boolean with an epollfd field in flow_common that serves dual
> > >>> purpose: zero indicates not registered (replacing in_epoll=false), non-zero  
> > >>
> > >> Don't use 0, since that's a valid fd.
> > >>  
> > >>> stores the actual epoll fd (replacing in_epoll=true).  
> > >>
> > >> I am a bit nervous about adding 31-bits to every flow, since I think
> > >> we're fairly close to a cacheline threshold.  
> > > 
> > > I mentioned to Laurent in our weekly call on Monday the same concern
> > > and that I would look into it (first step, I would say, actually check
> > > with pahole?) and, if it's an issue, into possible tricks to avoid
> > > making the struct too big. I still plan to do this.
> > 
> > I think it already doesn't fit in a cacheline:
> > 
> > struct flow_common {
> >          enum flow_state            state:8;              /*     0: 0  4 */
> >          enum flow_type             type:8;               /*     0: 8  4 */
> > 
> >          /* Bitfield combined with next fields */
> > 
> >          uint8_t                    pif[2];               /*     2     2 */
> >          struct flowside            side[2];              /*     4    72 */
> > 
> >          /* size: 76, cachelines: 2, members: 4 */
> >          /* last cacheline: 12 bytes */
> > };
> 
> Look at union flow, as we have an array of those, not flow_common. It's
> two cachelines at the moment, we shouldn't go beyond that, and keeping
> some margin would be nice as TCP might need some more bits
> eventually.

Right, I was unclear: it's going from 2 cachelines to 3 that I was
concerned about.

-- 
David Gibson (he or they)	| 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 --]