From patchwork Thu Jun 4 16:12:55 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Scott Feldman X-Patchwork-Id: 480790 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 1E9EA1401EF for ; Fri, 5 Jun 2015 02:14:25 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=ukOSY0m1; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752749AbbFDQNS (ORCPT ); Thu, 4 Jun 2015 12:13:18 -0400 Received: from mail-qc0-f181.google.com ([209.85.216.181]:36724 "EHLO mail-qc0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751677AbbFDQNQ (ORCPT ); Thu, 4 Jun 2015 12:13:16 -0400 Received: by qcxw10 with SMTP id w10so19682419qcx.3 for ; Thu, 04 Jun 2015 09:13:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=andxaw3HHN5q7eHjzSk6kN5kFedAOXWzCrEl+o7JzaY=; b=ukOSY0m1Ak26RA9ElBTDRqX+Zj5NhmADOTNdbC7KDBzGzZctqwtxw+Y3UwMheYkoA9 lwbp8hnz6w/BteF+84s1MNH5YE2ojFIXDsca/r96rJ4P50Pnnt6HEKgea+gDrezvoyou 7PB+JQTeO46enANNYcINMkO9SUcPboZ+jmQgJkhbrAlDTzMLoAZBaVqfoCciOCQEdVtT 1UsjhlvaDrK53Sv13FXvv3LPs7XfckRt8YV4qRrhAaGYWQtCg8q0XVsHkwTyiO39xC2q W1CH9NcMOgRNbcdK8M1U3h5Ug+JueyKWTMfVU9dPdfixep2UneOqsk6+TyIsqOfzapvU YMzA== X-Received: by 10.55.27.78 with SMTP id b75mr72730592qkb.7.1433434395615; Thu, 04 Jun 2015 09:13:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.96.76.199 with HTTP; Thu, 4 Jun 2015 09:12:55 -0700 (PDT) In-Reply-To: <20150604082045.GA7709@vergenet.net> References: <1433303008-32656-1-git-send-email-sfeldma@gmail.com> <20150603.233829.1362177764562847830.davem@davemloft.net> <20150604082045.GA7709@vergenet.net> From: Scott Feldman Date: Thu, 4 Jun 2015 09:12:55 -0700 Message-ID: Subject: Re: [PATCH net-next v2] rocker: move netevent neigh update to processes context To: Simon Horman Cc: David Miller , Netdev , =?UTF-8?B?SmnFmcOtIFDDrXJrbw==?= , Toshiaki Makita Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Jun 4, 2015 at 1:20 AM, Simon Horman wrote: > 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 >> > >> > v2: >> > >> > Changes based on review: >> > >> > - David Miller 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 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. Simon, just focusing on this rocker->neigh_tbl_next_index++ issue at the moment, I think the change below would make _rocker_neigh_add() safe to call without needing to put neigh_update into process context. It works because entry is stored between PREPARE and COMMIT, so once entry->index is assigned in PREPARE (under neigh_tbl_lock), it'll be re-used in COMMIT, even if the NONE thread also claims it's entry->index (also under neigh_tbl_lock). I know there are other issues you and Toshiaki Makita have pointed out, but let's see if we can peel the onion one layer at a time. --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/drivers/net/ethernet/rocker/rocker.c +++ b/drivers/net/ethernet/rocker/rocker.c @@ -2904,10 +2904,10 @@ static void _rocker_neigh_add(struct rocker *rocker, enum switchdev_trans trans, struct rocker_neigh_tbl_entry *entry) { - entry->index = rocker->neigh_tbl_next_index; + if (trans != SWITCHDEV_TRANS_COMMIT) + entry->index = rocker->neigh_tbl_next_index++; if (trans == SWITCHDEV_TRANS_PREPARE) return; - rocker->neigh_tbl_next_index++; entry->ref_count++; hash_add(rocker->neigh_tbl, &entry->entry, be32_to_cpu(entry->ip_addr));