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 13067C43381 for ; Fri, 15 Feb 2019 12:32:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 29CBA2192B for ; Fri, 15 Feb 2019 12:32:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2406112AbfBOMcU (ORCPT ); Fri, 15 Feb 2019 07:32:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34034 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725818AbfBOMcU (ORCPT ); Fri, 15 Feb 2019 07:32:20 -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 8C3951314E1; Fri, 15 Feb 2019 12:32: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 B7F795D706; Fri, 15 Feb 2019 12:32:16 +0000 (UTC) Date: Fri, 15 Feb 2019 13:32:10 +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 03/12] net: sched: flower: introduce reference counting for filters Message-ID: <20190215133210.64a99e93@redhat.com> In-Reply-To: References: <20190214074712.17846-1-vladbu@mellanox.com> <20190214074712.17846-4-vladbu@mellanox.com> <20190214213423.2260f5b9@redhat.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.39]); Fri, 15 Feb 2019 12:32:19 +0000 (UTC) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, 15 Feb 2019 11:22:45 +0000 Vlad Buslov wrote: > On Thu 14 Feb 2019 at 20:34, Stefano Brivio wrote: > > On Thu, 14 Feb 2019 09:47:03 +0200 > > Vlad Buslov wrote: > > > >> +static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp, > >> + unsigned long *handle) > >> +{ > >> + struct cls_fl_head *head = fl_head_dereference(tp); > >> + struct cls_fl_filter *f; > >> + > >> + rcu_read_lock(); > >> + /* don't return filters that are being deleted */ > >> + while ((f = idr_get_next_ul(&head->handle_idr, > >> + handle)) != NULL && > >> + !refcount_inc_not_zero(&f->refcnt)) > >> + ++(*handle); > > > > This... hurts :) What about: > > > > while ((f = idr_get_next_ul(&head->handle_idr, &handle))) { > > if (refcount_inc_not_zero(&f->refcnt)) > > break; > > ++(*handle); > > } > > > > ? > > I prefer to avoid using value of assignment as boolean and > non-structured jumps, when possible. In this case it seems OK either > way, but how about: > > for (f = idr_get_next_ul(&head->handle_idr, handle); > f && !refcount_inc_not_zero(&f->refcnt); > f = idr_get_next_ul(&head->handle_idr, handle)) > ++(*handle); Honestly, I preferred the original, this is repeating idr_get_next_ul() twice. Maybe, just: [...] struct idr *idr; [...] idr = &head->handle_idr; while ((f = idr_get_next_ul(idr, handle)) != NULL && !refcount_inc_not_zero(&f->refcnt)) ++(*handle); also rather ugly, but not entirely unreadable. I tried drafting a helper for this, but it just ends up hiding what this does. > >> @@ -1349,6 +1404,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, > >> err = -ENOBUFS; > >> goto errout_tb; > >> } > >> + refcount_set(&fnew->refcnt, 1); > >> > >> err = tcf_exts_init(&fnew->exts, TCA_FLOWER_ACT, 0); > >> if (err < 0) > >> @@ -1381,6 +1437,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb, > >> if (!tc_in_hw(fnew->flags)) > >> fnew->flags |= TCA_CLS_FLAGS_NOT_IN_HW; > >> > >> + refcount_inc(&fnew->refcnt); > > > > I guess I'm not getting the semantics but... why is it 2 now? > > As soon as fnew is inserted into head->handle_idr (one reference), it > becomes accessible to concurrent users, which means that it can be > deleted at any time. However, tp->change() returns a reference to newly > created filter to cls_api by assigning "arg" parameter to it (second > reference). After tp->change() returns, cls API continues to use fnew > and releases it with tfilter_put() when finished. I see, thanks for the explanation! -- Stefano