On Sun, Jun 26 2022, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
> On Sun, Jun 26, 2022 at 11:26:13PM +0200, Jeremie Courreges-Anglas wrote:
>> On Mon, Jun 27 2022, Vitaliy Makkoveev <m...@openbsd.org> wrote:
>> > On Sun, Jun 26, 2022 at 10:39:10PM +0200, Alexander Bluhm wrote:
>> >> On Sun, Jun 26, 2022 at 07:45:57PM +0300, Vitaliy Makkoveev wrote:
>> >> > This extra serialization is not required. In packet processing path we
>> >> > have shared netlock held, but we do read-only access on per session
>> >> > `flags' and `ifindex'. We always modify them from ioctl(2) path with
>> >> > exclusive netlock held. The rest of pipex(4) session is immutable or
>> >> > uses per-session locks.
>> >>
>> >> I was wondering about pipex_enable variable. It is documented as
>> >> protected by netlock.
>> >> net/pipex.c:int pipex_enable = 0; /* [N] */
>> >>
>> >> But I did not find NET_LOCK() in pipex_sysctl() or its callers.
>> >> Should we grab net lock there or in net_sysctl() in the write path?
>> >> Although only an integer is set atomically, it looks strange if
>> >> such a value is changed while we are in the middle of packet
>> >> processing.
>> >>
>> >
>> > I guess it should be declared as atomic. We use ifq_set_maxlen(...,1)
>> > hack so we don't cross netlock borders in pipex(4) output path, but it's
>> > expected we could cross them. We never check `pipex_enable' within
>> > (*if_qstart)() and we don't worry it's not serialized with the rest of
>> > stack. Also we will process already enqueued pipex(4) packets regardless
>> > on `pipex_enable' state.
>> >
>> > But this means we need to use local copy of `pipex_enable' within
>> > pppx_if_output(), otherwise we loose consistency.
>>
>> FWIW pipex_enable_local = READ_ONCE(pipex_enable) would prevent the
>> compiler from reading the global value twice.
>
> Or use atomic_load_int() to prevent over optimization.
READ_ONCE and atomic_load_int both use the volatile pointer dereference
trick to prevent reloading, so they should be equivalent for this use
case.
> I would prefer atomic_...() access for [A] variables.
... if you can live with the fact that some read accesses are done with
atomic_load_int and some are not, and write access isn't done with
atomic_store_int.
I'm not saying it's a problem in practice, just suggesting that
READ_ONCE() looks more appropriate here, you folks decide what suits you
best.
> bluhm
>
>> > Index: sys/net/if_pppx.c
>> > ===================================================================
>> > RCS file: /cvs/src/sys/net/if_pppx.c,v
>> > retrieving revision 1.116
>> > diff -u -p -r1.116 if_pppx.c
>> > --- sys/net/if_pppx.c 26 Jun 2022 15:50:21 -0000 1.116
>> > +++ sys/net/if_pppx.c 26 Jun 2022 21:14:02 -0000
>> > @@ -817,7 +817,9 @@ pppx_if_output(struct ifnet *ifp, struct
>> > struct pppx_if *pxi = (struct pppx_if *)ifp->if_softc;
>> > struct pppx_hdr *th;
>> > int error = 0;
>> > - int proto;
>> > + int pipex_enable_local, proto;
>> > +
>> > + pipex_enable_local = pipex_enable;
>> >
>> > NET_ASSERT_LOCKED();
>> >
>> > @@ -831,7 +833,7 @@ pppx_if_output(struct ifnet *ifp, struct
>> > if (ifp->if_bpf)
>> > bpf_mtap_af(ifp->if_bpf, dst->sa_family, m, BPF_DIRECTION_OUT);
>> > #endif
>> > - if (pipex_enable) {
>> > + if (pipex_enable_local) {
>> > switch (dst->sa_family) {
>> > #ifdef INET6
>> > case AF_INET6:
>> > @@ -856,7 +858,7 @@ pppx_if_output(struct ifnet *ifp, struct
>> > }
>> > *mtod(m, int *) = proto;
>> >
>> > - if (pipex_enable)
>> > + if (pipex_enable_local)
>> > error = if_enqueue(ifp, m);
>> > else {
>> > M_PREPEND(m, sizeof(struct pppx_hdr), M_DONTWAIT);
>> > Index: sys/net/pipex.c
>> > ===================================================================
>> > RCS file: /cvs/src/sys/net/pipex.c,v
>> > retrieving revision 1.138
>> > diff -u -p -r1.138 pipex.c
>> > --- sys/net/pipex.c 26 Jun 2022 15:50:21 -0000 1.138
>> > +++ sys/net/pipex.c 26 Jun 2022 21:14:02 -0000
>> > @@ -94,7 +94,7 @@ struct pool mppe_key_pool;
>> > * L pipex_list_mtx
>> > */
>> >
>> > -int pipex_enable = 0; /* [N] */
>> > +int pipex_enable = 0; /* [A] */
>> > struct pipex_hash_head
>> > pipex_session_list, /* [L] master session
>> > list */
>> > pipex_close_wait_list, /* [L] expired session
>> > list */
>> >
>>
>> --
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
