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. 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. 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? bluhm
