From: Simon Horman <simon.horman@netronome•com>
To: David Miller <davem@davemloft•net>
Cc: sfeldma@gmail•com, netdev@vger•kernel.org, jiri@resnulli•us,
makita.toshiaki@lab•ntt.co.jp
Subject: Re: [PATCH net-next v2] rocker: move netevent neigh update to processes context
Date: Thu, 4 Jun 2015 17:20:48 +0900 [thread overview]
Message-ID: <20150604082045.GA7709@vergenet.net> (raw)
In-Reply-To: <20150603.233829.1362177764562847830.davem@davemloft.net>
Hi Dave,
On Wed, Jun 03, 2015 at 11:38:29PM -0700, David Miller wrote:
> From: sfeldma@gmail•com
> Date: Tue, 2 Jun 2015 20:43:28 -0700
>
> > From: Scott Feldman <sfeldma@gmail•com>
> >
> > v2:
> >
> > Changes based on review:
> >
> > - David Miller <davem@davemloft•net> raise problem with system_wq not
> > preserving queue order to execution order. To fix, use driver-private
> > ordered workqueue to preserve ordering of queued work.
> >
> > - Jiri Pirko <jiri@resnulli•us> small change on kfree of work queue item.
> >
> > v1:
> >
> > In review of Simon's patchset "rocker: transaction fixes". it was noted
> > that rocker->neigh_tbl_next_index was unprotected in the call path below
> > and could race with other contexts calling rocker_port_ipv4_neigh():
>
> How it rocker->neigh_tbl_next_index not protected?
>
> rocker->neigh_tbl_lock is _always_ held when it is accessed.
>
> This patch, therefore, looks like completely unnecessary complexity
> to me. Furthermore, I would completely prefer if the operation stays
> completely synchronous to the call path where the neigh operation
> occurs rather than throwing it out to a workqueue.
What I was seeing is as follows:
1. rocker_port_ipv4_nh() is called via switchdev_port_obj_add()
with trans = SWITCHDEV_TRANS_PREPARE
2. rocker_port_ipv4_neigh() is called by rocker_neigh_update()
with trans = SWITCHDEV_TRANS_NONE.
The call chain goes up to arp_process() via neigh_update().
3. rocker_port_ipv4_nh() is called via switchdev_port_obj_add()
with trans = SWITCHDEV_TRANS_COMMIT
#1 and #2 are guarded by rtl across those calls but
#2 is not guarded by rtnl.
Inside both rocker_port_ipv4_nh() and rocker_port_ipv4_neigh()
neigh_tbl_lock lock is taken but it is not held across the
two calls to rocker_port_ipv4_nh within a single prepare->commit transaction.
I can double check that the above still occurs, but I'm not aware of any
recent changes that would cause it not to occur any more.
next prev parent reply other threads:[~2015-06-04 8:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-03 3:43 [PATCH net-next v2] rocker: move netevent neigh update to processes context sfeldma
2015-06-04 6:38 ` David Miller
2015-06-04 8:20 ` Simon Horman [this message]
2015-06-04 8:34 ` David Miller
2015-06-04 9:07 ` Simon Horman
2015-06-04 15:34 ` Toshiaki Makita
2015-06-04 18:48 ` David Miller
2015-06-04 16:12 ` Scott Feldman
2015-06-04 22:54 ` Simon Horman
2015-06-05 1:33 ` Scott Feldman
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=20150604082045.GA7709@vergenet.net \
--to=simon.horman@netronome$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jiri@resnulli$(echo .)us \
--cc=makita.toshiaki@lab$(echo .)ntt.co.jp \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sfeldma@gmail$(echo .)com \
/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