From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lennert Buytenhek Subject: Re: rcu locking issue in mpls output code? Date: Mon, 20 Jun 2016 09:30:55 +0300 Message-ID: <20160620063055.GR20238@wantstofly.org> References: <20160620004546.GP20238@wantstofly.org> <8d325ab7-bc46-dd69-288a-571d56efc545@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Roopa Prabhu , Robert Shearman , netdev@vger.kernel.org To: David Ahern Return-path: Received: from hmm.wantstofly.org ([138.201.34.84]:52682 "EHLO mail.wantstofly.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751687AbcFTGbW (ORCPT ); Mon, 20 Jun 2016 02:31:22 -0400 Content-Disposition: inline In-Reply-To: <8d325ab7-bc46-dd69-288a-571d56efc545@cumulusnetworks.com> Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Jun 19, 2016 at 08:19:20PM -0600, David Ahern wrote: > > diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c > > index fb31aa8..802956b 100644 > > --- a/net/mpls/mpls_iptunnel.c > > +++ b/net/mpls/mpls_iptunnel.c > > @@ -105,12 +105,15 @@ static int mpls_output(struct net *net, struct sock *sk, struct sk_buff *skb) > > bos = false; > > } > > > > + rcu_read_lock_bh(); > > if (rt) > > err = neigh_xmit(NEIGH_ARP_TABLE, out_dev, &rt->rt_gateway, > > skb); > > else if (rt6) > > err = neigh_xmit(NEIGH_ND_TABLE, out_dev, &rt6->rt6i_gateway, > > skb); > > + rcu_read_unlock_bh(); > > + > > if (err) > > net_dbg_ratelimited("%s: packet transmission failed: %d\n", > > __func__, err); > > > > I think those need to be added to neigh_xmit in the > > if (likely(index < NEIGH_NR_TABLES)) { > > } That'll force callers that don't need the extra protection (i.e. mpls_forward(), since that always runs from softirq and it's enough to protect the neigh state with rcu_read_lock() from softirq and we're already running under rcu_read_lock() when we get to neigh_xmit()) to eat the useless overhead of an extra rcu_read_{,un}lock_bh() pair, but sure, functionally that's correct, I think, and in my workload I don't care about MPLS forwarding performance anyway. ;-) Want me to send a patch moving it to neigh_xmit() ? Thank you for having a look! Cheers, Lennert