On Fri, Oct 06, 2023 at 02:50:21PM +0200, Alexander Bluhm wrote: > On Fri, Oct 06, 2023 at 12:45:54PM +0300, Vitaliy Makkoveev wrote: > > On Thu, Oct 05, 2023 at 10:42:25PM +1000, David Gwynne wrote: > > > > On 5 Oct 2023, at 21:50, Vitaliy Makkoveev <m...@openbsd.org> wrote: > > > > > > > > I don't like this unlocked if_flags check we have in ifq_start_task(). > > > > Guess READ_ONCE() is much better to load value. > > > > > > there are no inconsistent intermediate values you can read from if_flags. > > > are you worried the compiler will load the value again and maybe we'll > > > get a different state the second time? > > > > To enforce loading it from memory instead of cache. But may be I'm > > wrong. > > READ_ONCE() does not address cache coherency problmes. > > Just reading an int value that an other processor can modify is > wrong. The compiler can optimize it into multiple reads with > inconsistent values, witten by the other CPU. This is prevented > by READ_ONCE(). > > When using READ_ONCE() you must be sure that cache coherency is not > a problem or that you have some memory barriers in place. I added > READ_ONCE() to some queue macros recently to fix obvious missuse. > That does not mean it is sufficient. >
Thanks for explanation. > Accessing ifp->if_flags is a different story. As it is marked as > [N] you need exclusive netlock to write and shared netlock to read > to be on the safe side. A lot of your code just reads it without > caring about locks. This works most of the time. > Not my code. However these IFF_UP and IFF_RUNNING reads and modifications are within kernel locked chunks of drivers and kernel locked ifioctl() chunk. Mostly, this is configuration path. In normal condition these flags should be modified a very little times, mostly once. So this works... Some times ago, I privately pointed this and proposed to modify if_flags atomically. May be it's time to rethink this. > I this case, I don't know. Would it be a problem if ISSET(ifp->if_flags), > ifq_empty(ifq), and ifq_is_oactive(ifq) would be executed in different > order? > No. They all do simply load, compare and ored the result, no difference what condition was the reason to return.
