Re: [RESEND 3/5] [NET]: Protocol Independant Policy Routing Rules Framework

[Martin Josefsson <gandalf@wlug.westbo.se>]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

On Fri, 2006-07-28 at 00:58 +0200, Patrick McHardy wrote:

> > +int fib_rules_lookup(struct fib_rules_ops *ops, struct flowi *fl,
> > +		     int flags, struct fib_lookup_arg *arg)
> > +{
> > +	struct fib_rule *rule;
> > +	int err;
> > +
> > +	rcu_read_lock();
> > +
> > +	list_for_each_entry(rule, ops->rules_list, list) {
> 
> Shouldn't that be list_for_each_entry_rcu?

Yes that's correct, it should.

> > +		if (rule->ifname[0] && (rule->ifindex != fl->iif))
> > +			continue;
> > +
> > +		if (!ops->match(rule, fl, flags))
> > +			continue;
> > +
> > +		rcu_read_unlock();
> > +
> > +		err = ops->action(rule, fl, flags, arg);
> > +		if (err != -EAGAIN) {
> > +			fib_rule_get(rule);
> > +			arg->rule = rule;
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	err = -ENETUNREACH;
> > +out:
> > +	rcu_read_unlock();
> 
> rcu_read_unlock might get called multiple times in the list iteration
> and once again here.

Yes, the rcu_read_unlock() in the list iteration is misplaced, it
shouldn't be there. Besides the unbalanced lock/unlocks it suffers from
the general issue described below

As a somewhat related note, I've just digged a bit through RCU land,
talked to dipankar and mckenney, and discovered that rcu_read_lock() /
rcu_read_unlock() aren't strictly needed in softirqs since preempt is
already disabled in softirqs. This means that you can use the result of
the rcu read-side critical outside of the rcu_read_lock() /
rcu_read_unlock() section. BUT this changes with the -rt kernel where
softirqs are preemptable and where rcu_read_lock() / rcu_read_unlock()
doesn't disable/enable preempt anymore, which means the rcu read-side
critical section is also preemptable. This means that we can get
preempted in the read-side critical section but the resulting grace
period won't occur until rcu_read_unlock() is called, which means that
using results of an read-side critical section outside of the critical
section is just not going to work in softirqs in -rt kernels.
I'm sure Ingo has reviewed the RCU usage in softirqs but I don't know if
there's been any changes in this area after his review.

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]