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. >
Such case was already discussed, but as I remember the opinions were different. Does something changed what I missed? The READ_ONCE() code doesn't changed from that time. Also if I should, I prefer to use atomic_load_int(9) because it documented and more consistent with our atomic API.
