From: David Ahern <dsa@cumulusnetworks•com>
To: Julian Anastasov <ja@ssi•bg>
Cc: netdev@vger•kernel.org
Subject: Re: [PATCH v2 net-next] net: ipv4: Consider unreachable nexthops in multipath routes
Date: Fri, 1 Apr 2016 08:39:30 -0600 [thread overview]
Message-ID: <56FE8822.3080900@cumulusnetworks.com> (raw)
In-Reply-To: <alpine.LFD.2.11.1604011005480.2190@ja.home.ssi.bg>
On 4/1/16 2:09 AM, Julian Anastasov wrote:
>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>> index d97268e8ff10..6d423faff0ce 100644
>> --- a/net/ipv4/fib_semantics.c
>> +++ b/net/ipv4/fib_semantics.c
>> @@ -1559,17 +1559,45 @@ int fib_sync_up(struct net_device *dev, unsigned int nh_flags)
>> }
>>
>> #ifdef CONFIG_IP_ROUTE_MULTIPATH
>> +static bool fib_good_nh(const struct fib_nh *nh, struct net_device *dev)
>> +{
>> + struct neighbour *n = NULL;
>> + int state = NUD_NONE;
>
> Looks like we can do it even better.
> If we use NUD_REACHABLE here...
>
>> +
>> + if (nh->nh_scope == RT_SCOPE_LINK) {
>> + rcu_read_lock_bh();
>> +
>> + n = __neigh_lookup_noref(&arp_tbl, &nh->nh_gw, dev);
>> + if (n)
>> + state = n->nud_state;
>> +
>> + rcu_read_unlock_bh();
>> + }
>> +
>> + /* outside of rcu locking using n only as a boolean
>> + * on whether a neighbor entry existed
>> + */
>> + if (!n || (state & NUD_VALID))
>
> then check for '!n' is not needed. For bool type
> 'return state & NUD_VALID;' should work.
good simplification.
>
>> + return true;
>> +
>> + return false;
>> +}
>>
>> void fib_select_multipath(struct fib_result *res, int hash)
>> {
>> struct fib_info *fi = res->fi;
>> + struct net_device *dev = fi->fib_dev;
>> + struct net *net = fi->fib_net;
>>
>> for_nexthops(fi) {
>> if (hash > atomic_read(&nh->nh_upper_bound))
>> continue;
>>
>> - res->nh_sel = nhsel;
>> - return;
>> + if (!net->ipv4.sysctl_fib_multipath_use_neigh ||
>> + fib_good_nh(nh, dev)) {
>
> This dev is from first nexthop. Better fib_good_nh
> to use nh->nh_dev instead, it is present for all nexthops
> in multipath route. We should not copy the bugs from
> fib_detect_death.
ack. Will fix.
>
>> + res->nh_sel = nhsel;
>> + return;
>> + }
>> } endfor_nexthops(fi);
>
> So, you dropped the idea to give full chance for
> fallback? Now if last nexthop fails we do not fallback at all.
> We promised to prefer reachable nexthops.
I'll save the first nhsel not selected and if all fails return it. That
keeps the fallback inline with what happens today.
prev parent reply other threads:[~2016-04-01 14:39 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-31 22:24 [PATCH v2 net-next] net: ipv4: Consider unreachable nexthops in multipath routes David Ahern
2016-04-01 8:09 ` Julian Anastasov
2016-04-01 14:39 ` David Ahern [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56FE8822.3080900@cumulusnetworks.com \
--to=dsa@cumulusnetworks$(echo .)com \
--cc=ja@ssi$(echo .)bg \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox