From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 26B60C43381 for ; Mon, 4 Mar 2019 23:57:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 003DC206B8 for ; Mon, 4 Mar 2019 23:57:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726820AbfCDX5U (ORCPT ); Mon, 4 Mar 2019 18:57:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55366 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726066AbfCDX5T (ORCPT ); Mon, 4 Mar 2019 18:57:19 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 108C83082AF0; Mon, 4 Mar 2019 23:57:19 +0000 (UTC) Received: from localhost (ovpn-200-19.brq.redhat.com [10.40.200.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AABEC5D6B3; Mon, 4 Mar 2019 23:57:16 +0000 (UTC) Date: Tue, 5 Mar 2019 00:57:11 +0100 From: Stefano Brivio To: Vlad Buslov Cc: netdev@vger.kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net Subject: Re: [PATCH net-next v2 00/12] Refactor flower classifier to remove dependency on rtnl lock Message-ID: <20190305005711.0dba1b69@redhat.com> In-Reply-To: <20190227101226.26196-1-vladbu@mellanox.com> References: <20190227101226.26196-1-vladbu@mellanox.com> Organization: Red Hat MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Mon, 04 Mar 2019 23:57:19 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, 27 Feb 2019 12:12:14 +0200 Vlad Buslov wrote: > Currently, all netlink protocol handlers for updating rules, actions and > qdiscs are protected with single global rtnl lock which removes any > possibility for parallelism. This patch set is a third step to remove > rtnl lock dependency from TC rules update path. > > Recently, new rtnl registration flag RTNL_FLAG_DOIT_UNLOCKED was added. > TC rule update handlers (RTM_NEWTFILTER, RTM_DELTFILTER, etc.) are > already registered with this flag and only take rtnl lock when qdisc or > classifier requires it. Classifiers can indicate that their ops > callbacks don't require caller to hold rtnl lock by setting the > TCF_PROTO_OPS_DOIT_UNLOCKED flag. The goal of this change is to refactor > flower classifier to support unlocked execution and register it with > unlocked flag. > > This patch set implements following changes to make flower classifier > concurrency-safe: > > - Implement reference counting for individual filters. Change fl_get to > take reference to filter. Implement tp->ops->put callback that was > introduced in cls API patch set to release reference to flower filter. > > - Use tp->lock spinlock to protect internal classifier data structures > from concurrent modification. > > - Handle concurrent tcf proto deletion by returning EAGAIN, which will > cause cls API to retry and create new proto instance or return error > to the user (depending on message type). > > - Handle concurrent insertion of filter with same priority and handle by > returning EAGAIN, which will cause cls API to lookup filter again and > process it accordingly to netlink message flags. > > - Extend flower mask with reference counting and protect masks list with > masks_lock spinlock. > > - Prevent concurrent mask insertion by inserting temporary value to > masks hash table. This is necessary because mask initialization is a > sleeping operation and cannot be done while holding tp->lock. > > Both chain level and classifier level conflicts are resolved by > returning -EAGAIN to cls API that results restart of whole operation. > This retry mechanism is a result of fine-grained locking approach used > in this and previous changes in series and is necessary to allow > concurrent updates on same chain instance. Alternative approach would be > to lock the whole chain while updating filters on any of child tp's, > adding and removing classifier instances from the chain. However, since > most CPU-intensive parts of filter update code are specifically in > classifier code and its dependencies (extensions and hw offloads), such > approach would negate most of the gains introduced by this change and > previous changes in the series when updating same chain instance. > > Tcf hw offloads API is not changed by this patch set and still requires > caller to hold rtnl lock. Refactored flower classifier tracks rtnl lock > state by means of 'rtnl_held' flag provided by cls API and obtains the > lock before calling hw offloads. Following patch set will lift this > restriction and refactor cls hw offloads API to support unlocked > execution. > > With these changes flower classifier is safely registered with > TCF_PROTO_OPS_DOIT_UNLOCKED flag in last patch. > > Changes from V1 to V2: > - Extend cover letter with explanation about retry mechanism. > - Rebase on current net-next. > - Patch 1: > - Use rcu_dereference_raw() for tp->root dereference. > - Update comment in fl_head_dereference(). > - Patch 2: > - Remove redundant check in fl_change error handling code. > - Add empty line between error check and new handle assignment. > - Patch 3: > - Refactor loop in fl_get_next_filter() to improve readability. > - Patch 4: > - Refactor __fl_delete() to improve readability. > - Patch 6: > - Fix comment in fl_check_assign_mask(). > - Patch 9: > - Extend commit message. > - Fix error code in comment. > - Patch 11: > - Fix fl_hw_replace_filter() to always release rtnl lock in error > handlers. > - Patch 12: > - Don't take rtnl lock before calling __fl_destroy_filter() in > workqueue context. > - Extend commit message with explanation why flower still takes rtnl > lock before calling hardware offloads API. FWIW, Reviewed-by: Stefano Brivio -- Stefano