fib_trie: more whitespace cleanup

Remove extra blank lines.

Signed-off-by Stephen Hemminger <[email protected]>

Since fib_route_seq_show now uses hlist_for_each_entry(), the leaf info can not be NULL.

Signed-off-by Stephen Hemminger <[email protected]>

Since fib dump (via netlink) holds the RTNL mutex, it is unnecessary to use RCU, and it is impossible to get truncated (-EBUSY) result.

Signed-off-by Stephen Hemminger <[email protected]>

From: Stephen Hemminger <[email protected]>
Date: Wed, 23 Jan 2008 14:48:47 -0800

Quoted 5 lines

Since fib dump (via netlink) holds the RTNL mutex, it is unnecessary to use RCU, and it is impossible to get truncated (-EBUSY) result.

Signed-off-by Stephen Hemminger <[email protected]>

You tested this patch, right? :-/

The whole reason we need the nlk->cb[] state is to hold things across multiple recvmsg() calls that might be necessary to obtain the full dump.

rtnetlink goes:

	rtnl_lock();
	netlink_rcv_skb(skb, &rtnetlink_rcv_msg);

...
static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) {
...

	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
		struct sock *rtnl;
		rtnl_dumpit_func dumpit;

		dumpit = rtnl_get_dumpit(family, type);
		if (dumpit == NULL)
			return -EOPNOTSUPP;

		__rtnl_unlock();
		rtnl = net->rtnl;
		err = netlink_dump_start(rtnl, skb, nlh, dumpit, NULL);
		rtnl_lock();
		return err;

(NOTE: Drops RTNL semaphore for netlink_dump_start() call)

...
int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
struct nlmsghdr *nlh,
int (*dump)(struct sk_buff *skb,
struct netlink_callback *),
int (*done)(struct netlink_callback *))
{
...

	cb->dump = dump;
	cb->done = done;
	cb->nlh = nlh;
	atomic_inc(&skb->users);
	cb->skb = skb;
 ...
	mutex_lock(nlk->cb_mutex);
 ...
	nlk->cb = cb;
	mutex_unlock(nlk->cb_mutex);

	netlink_dump(sk);

...

static int netlink_dump(struct sock *sk)
{
...

	mutex_lock(nlk->cb_mutex);
 ...
	len = cb->dump(skb, cb);

	if (len > 0) {
		mutex_unlock(nlk->cb_mutex);
		skb_queue_tail(&sk->sk_receive_queue, skb);
		sk->sk_data_ready(sk, len);
		return 0;
	}

(NOTE: Therefore cb->dump() runs without RTNL semaphore held)

...
static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len, int flags)
{
...

	if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2)
		netlink_dump(sk);

...

Therefore, that RTNL assertion you added should have triggered on any dump you may have tried since ->dump() is always invoked without the RTNL semaphore since rtnetlink drops it around the ->dump() call and the call chain for this fib_trie cause would be:

inet_dump_fib()
fn_trie_dump()

and nothing in that code path retakes the RTNL semaphore.

Quoted 1 line · David Miller

and nothing in that code path retakes the RTNL semaphore.

Actually we're always holding the rtnl during dumps, nlk->cb_mutex points to rtnl_mutex in case of rtnetlink. It used to be held only during the first ->dump invocation and not on continuations, but I changed this a few versions ago.

From: Patrick McHardy <[email protected]>
Date: Thu, 24 Jan 2008 07:41:08 +0100

Quoted 9 lines

Quoted 2 lines · David Miller

and nothing in that code path retakes the RTNL semaphore.

Actually we're always holding the rtnl during dumps, nlk->cb_mutex points to rtnl_mutex in case of rtnetlink. It used to be held only during the first ->dump invocation and not on continuations, but I changed this a few versions ago.

My bad. Thanks for the correction Patrick.

But continuations can occur on subsequent recvmsg() calls, does it return to userspace with the mutex held? If so I'm pretty sure that's not allowed.

Quoted 13 lines · David Miller

From: Patrick McHardy <[email protected]>
Date: Thu, 24 Jan 2008 07:41:08 +0100

Quoted 9 lines

Quoted 2 lines · David Miller

and nothing in that code path retakes the RTNL semaphore.

Actually we're always holding the rtnl during dumps, nlk->cb_mutex points to rtnl_mutex in case of rtnetlink. It used to be held only during the first ->dump invocation and not on continuations, but I changed this a few versions ago.

Quoted 5 lines

My bad. Thanks for the correction Patrick.

But continuations can occur on subsequent recvmsg() calls, does it return to userspace with the mutex held? If so I'm pretty sure that's not allowed.

No, the mutex is dropped between different ->dump invocations.

Quoted 96 lines · David Miller

From: Stephen Hemminger <[email protected]>
Date: Wed, 23 Jan 2008 14:48:47 -0800

Quoted 5 lines

Since fib dump (via netlink) holds the RTNL mutex, it is unnecessary to use RCU, and it is impossible to get truncated (-EBUSY) result.

Signed-off-by Stephen Hemminger <[email protected]>

You tested this patch, right? :-/

The whole reason we need the nlk->cb[] state is to hold things across multiple recvmsg() calls that might be necessary to obtain the full dump.

rtnetlink goes:

    rtnl_lock();
    netlink_rcv_skb(skb, &rtnetlink_rcv_msg);

...
static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) {
...

    if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
        struct sock *rtnl;
        rtnl_dumpit_func dumpit;

        dumpit = rtnl_get_dumpit(family, type);
        if (dumpit == NULL)
            return -EOPNOTSUPP;

        __rtnl_unlock();
        rtnl = net->rtnl;
        err = netlink_dump_start(rtnl, skb, nlh, dumpit, NULL);
        rtnl_lock();
        return err;

(NOTE: Drops RTNL semaphore for netlink_dump_start() call)

...
int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
struct nlmsghdr *nlh,
int (*dump)(struct sk_buff *skb,
struct netlink_callback *),
int (*done)(struct netlink_callback *))
{
...

    cb->dump = dump;
    cb->done = done;
    cb->nlh = nlh;
    atomic_inc(&skb->users);
    cb->skb = skb;
 ...
    mutex_lock(nlk->cb_mutex);
 ...
    nlk->cb = cb;
    mutex_unlock(nlk->cb_mutex);

    netlink_dump(sk);

...

static int netlink_dump(struct sock *sk)
{
...

    mutex_lock(nlk->cb_mutex);
 ...
    len = cb->dump(skb, cb);

    if (len > 0) {
        mutex_unlock(nlk->cb_mutex);
        skb_queue_tail(&sk->sk_receive_queue, skb);
        sk->sk_data_ready(sk, len);
        return 0;
    }

(NOTE: Therefore cb->dump() runs without RTNL semaphore held)

...
static int netlink_recvmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len,
int flags)
{
...

    if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2)
        netlink_dump(sk);

...

Therefore, that RTNL assertion you added should have triggered on any dump you may have tried since ->dump() is always invoked without the RTNL semaphore since rtnetlink drops it around the ->dump() call and the call chain for this fib_trie cause would be:

inet_dump_fib()
fn_trie_dump()

Quoted 1 line

and nothing in that code path retakes the RTNL semaphore.

Actually we're always holding the rtnl during dumps, nlk->cb_mutex points to rtnl_mutex in case of rtnetlink. It used to be held only during the first ->dump invocation and not on continuations, but I changed this a few versions
ago.

Yes, I tested, no the assert didn't hit... Actually, the reason I went down this path was
because I couldn't trigger -EBUSY with concurrent updates.

P.s: I checked and Quagga handles -EAGAIN, so if you need an error code that would be the one to use.

Normally during a dump the key of the last dumped entry is used for continuation, but since lock is dropped it might be lost. In that case fallback to the old counter based N^2 behaviour. This means the dump will end up skipping some routes which matches what FIB_HASH does.

Signed-off-by Stephen Hemminger <[email protected]>

Quoted 6 lines · Stephen Hemminger

Normally during a dump the key of the last dumped entry is used for continuation, but since lock is dropped it might be lost. In that case fallback to the old counter based N^2 behaviour. This means the dump will end up skipping some routes which matches what FIB_HASH does.

Signed-off-by Stephen Hemminger <[email protected]>

...

Quoted 6 lines

net/ipv4/fib_trie.c +1 -0

@@ -1918,35 +1931,37 @@ static int fn_trie_dump(struct fib_table
 	struct leaf *l;
 	struct trie *t = (struct trie *) tb->tb_data;
 	t_key key = cb->args[2];
+	int count = cb->args[3];
 

net/ipv4/fib_trie.c static int fn_trie_dump(struct fib_table · line 1936

Quoted 1 line

net/ipv4/fib_trie.c +0 -0

 	rcu_read_lock();

Sorry, but I lost the point: is rtnl held or not held here at the moment? If held, how this rcu_read_lock can help? Maybe some additional comment in the code?

Thanks,
Jarek P.

2. RCU is unnecessary here because of use of RTNL. I am going to defer on this till after #1. That patch is much less important.

Normally during a dump the key of the last dumped entry is used for continuation, but since lock is dropped it might be lost. In that case fallback to the old counter based N^2 behaviour. This means the dump will end up skipping some routes which matches what FIB_HASH does.

Signed-off-by Stephen Hemminger <[email protected]>

Increase version to reflect recent changes.

Signed-off-by Stephen Hemminger <[email protected]>