On Wed, 26 Nov 2014, Florian Westphal wrote:
> Jozsef Kadlecsik <kadlec@xxxxxxxxxxxxxxxxx> wrote:
> > On Wed, 26 Nov 2014, Pablo Neira Ayuso wrote:
> >
> > > > diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> > > > index f1606fa..418d360 100644
> > > > --- a/include/linux/netfilter/ipset/ip_set.h
> > > > +++ b/include/linux/netfilter/ipset/ip_set.h
> > > > @@ -113,10 +113,10 @@ struct ip_set_comment {
> > > > };
> > > >
> > > > struct ip_set_skbinfo {
> > > > - u32 skbmark;
> > > > - u32 skbmarkmask;
> > > > - u32 skbprio;
> > > > - u16 skbqueue;
> > > > + u32 __rcu skbmark;
> > > > + u32 __rcu skbmarkmask;
> > > > + u32 __rcu skbprio;
> > > > + u16 __rcu skbqueue;
> > > > };
>
> This looks weird...
> What is rcu protecting here?
>
> > > > +#define IP_SET_RCU_ASSIGN(ptr, value) \
> > > > +do { \
> > > > + smp_wmb(); \
>
> Whats this barrier for?
>
> > > > + *(ptr) = value; \
> > > > +} while (0)
>
> > > > +static inline void
> > > > +ip_set_rcu_assign_ulong(unsigned long *v, unsigned long value)
> > > > +{
> > > > + IP_SET_RCU_ASSIGN(v, value);
> > > > +}
> > > > +
> > > > +static inline void
> > > > +ip_set_rcu_assign_u32(u32 *v, u32 value)
> > > > +{
> > > > + IP_SET_RCU_ASSIGN(v, value);
> > > > +}
> > > > +
> > > > +static inline void
> > > > +ip_set_rcu_assign_u16(u16 *v, u16 value)
> > > > +{
> > > > + IP_SET_RCU_ASSIGN(v, value);
> > > > +}
> > > > +
> > > > +static inline void
> > > > +ip_set_rcu_assign_u8(u8 *v, u8 value)
> > > > +{
> > > > + IP_SET_RCU_ASSIGN(v, value);
> > > > +}
>
> These macros look dodgy... Why are they needed?
>
> > - elem extensions: the manipulations of the extensions are
> > either atomic operations or integer settings. The inline functions
> > above provide those RCU safe integer settings: they are the same
> > as rcu_assign_pointer(), but that one can be used for pointers only.
>
> Yes, because it doesn't make sense to me to have something NOT for
> pointers.
>
> rcu_assign_pointer() is there to make sure that when you "publish" the
> memory whose address is given, all the content is visible to all the
> cpus, this is also why it has the barrier.
>
> I don't see why you'd need a special function otherwise, unless you
> want some explicit documentation as to where you're changing/updating
> content of structures protected by rcu. But, in those sections you
> already need to hold some type of lock, so I don't think its needed.
Hm, you are both right - I clean up this part as it's totally unnecessary,
these are not pointers, even indirectly.
Thanks!
Best regards,
Jozsef
-
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxxxxxx
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
