On Wed, May 22, 2013 at 05:07:07PM +0400, Roman Gushchin wrote:

On 22.05.2013 16:30, Eric Dumazet wrote:

quoted

On Wed, 2013-05-22 at 15:58 +0400, Roman Gushchin wrote:

quoted

+/*
+ * Same as ACCESS_ONCE(), but used for accessing field of a structure.
+ * The main goal is preventing compiler to store &ptr->field in a register.

But &ptr->field is a constant during the whole duration of
udp4_lib_lookup2() and could be in a register, in my case field is at
offset 0, and ptr is a parameter (so could be in a 'register')

The bug you found is that compiler caches the indirection  (ptr->field)
into a register, not that compiler stores &ptr->field into a register.

quoted

+ */
+#define ACCESS_FIELD_ONCE(PTR, FIELD) (((volatile typeof(*PTR) *)PTR)->FIELD)
+

Here we force the compiler to consider ptr as volatile, but semantically
it is not required in rcu_dereference(ptr->field)

Actually, we need to mark an "address of a place" where the field value is
located as volatile before dereferencing. I have no idea how to do it in another way,
except using multiple casts and offsetof's, but, IMHO, it will be even more complex:
	ACCESS_ONCE(typeof(&ptr->field)((char*)ptr + offsetof(typeof(*ptr), field)))

Why not just ACCESS_ONCE(ptr->field)?  Or if it is the thing that
ptr->field points to that is subject to change, ACCESS_ONCE(*ptr->field)?

Or rcu_dereference(ptr->field), as appropriate?

							Thanx, Paul

quoted

We want field to be reloaded, not ptr.

So yes, the patch appears to fix the bug, but it sounds not logical to
me.

May be we can enhance it by providing better/more detailed comments here?
Have you any suggestions?

Thanks,
Roman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/