From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: pf inpcb link mutex
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: "Lorenz (xha)" <me@xha.li>, tech@openbsd.org
Date: Mon, 1 Jan 2024 19:10:37 +0100

Hello,

On Mon, Jan 01, 2024 at 07:06:13PM +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> On Mon, Jan 01, 2024 at 05:57:58PM +0100, Alexander Bluhm wrote:
> > On Mon, Jan 01, 2024 at 05:45:51PM +0100, Lorenz (xha) wrote:
> > > On Mon, Jan 01, 2024 at 12:20:09AM +0100, Alexander Bluhm wrote:
> > > > +	mtx_leave(&pf_inp_mtx);
> > > > +        if (sk == NULL)
> > > > +                return;
> > > 
> > > whitespace before the "if"?
> > 
> > fixed diff
> 
>     change looks correct to me. Though I'm bit concerned about
>     design choice made in this diff. see further below.
> > 
> > Index: net/pf.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> > diff -u -p -r1.1190 pf.c
> > --- net/pf.c	28 Dec 2023 16:21:08 -0000	1.1190
> > +++ net/pf.c	1 Jan 2024 16:56:02 -0000
> > @@ -112,6 +112,8 @@ struct pf_queuehead	*pf_queues_inactive;
> >  
> >  struct pf_status	 pf_status;
> >  
> > +struct mutex		 pf_inp_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
> 
>     I would prefer to move this mutex inside the struct inpcb. I know it
>     makes structure larger but it looks odd to me using global mutex to
>     update members which are local to structure.
> 
>     the functions in pf(4) then can manipulate the inpcb::mtx by
>     dedicated functions:
> 	inpcb_lock(inp);
> 	inpcb_unlock(inp);
> 
>     just a thought. sure we can make such change as a follow up commit
>     on top of your diff.
> 

    in other words: can we use inpcb::inp_mtx in your diff? this way we
    can avoid introduction of global mutex.

thanks and
regards
sashan