On Thu, Dec 10, 2020 at 11:26:16AM -0600, Scott Cheloha wrote:
> Hi,
>
> Before converting bpf(4) from using ticks to using real units of time
> we need to serialize BIOCGRTIMEOUT and BIOCSRTIMEOUT. Neither
> operation is atomic so we need to use the per-descriptor mutex when
> reading or writing the bd_rtout member.
>
> While here we can start annotating the locking for struct members in
> bpfdesc.h, too.
>
> ok?
> Index: bpf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/bpf.c,v
> retrieving revision 1.193
> diff -u -p -r1.193 bpf.c
> --- bpf.c 4 Nov 2020 04:40:13 -0000 1.193
> +++ bpf.c 10 Dec 2020 17:24:43 -0000
> @@ -873,9 +873,11 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
> break;
> }
> rtout += tv->tv_usec / tick;
> + mtx_enter(&d->bd_mtx);
> d->bd_rtout = rtout;
> if (d->bd_rtout == 0 && tv->tv_usec != 0)
> d->bd_rtout = 1;
This code could be refactored to write bd_rtout only once.
if (rtout == 0 && tv->tv_usec != 0)
rtout = 1;
d->bd_rtout = rtout;
or using WRITE_ONCE
WRITE_ONCE(d->bd_rtout, rtout);
This way the mutex would no longer be required since this is an atomic
update.
> + mtx_leave(&d->bd_mtx);
> break;
> }
>
> @@ -886,8 +888,10 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
> {
> struct timeval *tv = (struct timeval *)addr;
>
> + mtx_enter(&d->bd_mtx);
> tv->tv_sec = d->bd_rtout / hz;
> tv->tv_usec = (d->bd_rtout % hz) * tick;
Same here using a local rtout variable:
rtout = READ_ONCE(d->bd_rtout);
tv->tv_sec = rtout / hz;
tv->tv_usec = (rtout % hz) * tick;
Again this would be an atomic operation and the lock would not be needed
anymore.
I guess moving forward bd_rtout will be changed since it is tick based so
maybe then the update can no longer be made in an atomic fashion.
> + mtx_leave(&d->bd_mtx);
> break;
> }
>
> Index: bpfdesc.h
> ===================================================================
> RCS file: /cvs/src/sys/net/bpfdesc.h,v
> retrieving revision 1.41
> diff -u -p -r1.41 bpfdesc.h
> --- bpfdesc.h 13 May 2020 21:34:37 -0000 1.41
> +++ bpfdesc.h 10 Dec 2020 17:24:43 -0000
> @@ -42,6 +42,13 @@
>
> #ifdef _KERNEL
>
> +/*
> + * Locks used to protect struct members in this file:
> + *
> + * I immutable after initialization
> + * m the per-descriptor mutex (bpf_d.bd_mtx)
> + */
> +
> struct bpf_program_smr {
> struct bpf_program bps_bf;
> struct smr_entry bps_smr;
> @@ -72,7 +79,7 @@ struct bpf_d {
> int bd_in_uiomove; /* for debugging purpose */
>
> struct bpf_if *bd_bif; /* interface descriptor */
> - u_long bd_rtout; /* Read timeout in 'ticks' */
> + u_long bd_rtout; /* [m] Read timeout in 'ticks' */
> u_long bd_rdStart; /* when the read started */
> int bd_rnonblock; /* true if nonblocking reads are set */
> struct bpf_program_smr
>
--
:wq Claudio
