Re: [PATCH v2] 9p/trans_fd: mark concurrent read and writes to p9_conn->err

[Dominique Martinet]

Ignacio Encinas Rubio wrote on Tue, Mar 18, 2025 at 10:21:52PM +0100:
> Trimming CC to avoid spamming people (I hope that's ok)

Probably fine :)

> > This is weird... I'll double check because it shouldn't generate the
> > same code as far as I know.
> 
> I had a bit of time to check this. I understood you said that (A)
> 
> 	err = READ_ONCE(m->err);
> 	if (err < 0) {
> 		spin_unlock(&m->req_lock);
> 		return READ_ONCE(m->err);
> 	}
> 
> compiles to the same thing as (B)
> 
> 	err = READ_ONCE(m->err);
> 	if (err < 0) {
> 		spin_unlock(&m->req_lock);
> 		return err;
> 	}
> 
> if you didn't say this, just ignore this email :).

Right, I checked by looking at `objdump -D` output after rebuilding the
.o.
I've just double-checked and these two are identical:
```
err = READ_ONCE(m->err);
if (err < 0) {
    spin_unlock(&m->req_lock);
    return m->err;
}
```
```
err = READ_ONCE(m->err);
if (err < 0) {
    spin_unlock(&m->req_lock);
    return READ_ONCE(m->err);
}
```
but now I'm double-checking returning the local variable
```
```
err = READ_ONCE(m->err);
if (err < 0) {
    spin_unlock(&m->req_lock);
    return err;
}
```
is properly different (and is the right thing to do), so I must have
checked the wrong .o, sorry for the confusion.
(that or the minor gcc upgrade I did this morning change this, but I'd
rather inclined to think I screwed up...)

> With gcc (GCC) 14.2.1 20250110 (Red Hat 14.2.1-7) I'm seeing a difference:
> (A) performs another memory read after the spinlock has been unlocked
> while (B) reuses the value from the register. If you're using an old GCC
> it might have bugs. I can't recall where exactly but I have seen links
> to GCC bugs regarding this issues somewhere (LWN posts or kernel docs?)

Right, I'm seeing one less mov as well

So, yeah, v3 with this would be great :)
I don't have a strong opinion on the READ_ONCE within the lock, so if
you think it's clearer without it then I'm fine with that.

Thanks for taking the time to check!
-- 
Dominique Martinet | Asmadeus