From patchwork Thu Apr 20 11:35:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jiri Pirko X-Patchwork-Id: 752750 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 3w7xdJ0lGKz9s65 for ; Thu, 20 Apr 2017 21:35:48 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=resnulli-us.20150623.gappssmtp.com header.i=@resnulli-us.20150623.gappssmtp.com header.b="lzWGTeU0"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964833AbdDTLfq (ORCPT ); Thu, 20 Apr 2017 07:35:46 -0400 Received: from mail-wr0-f194.google.com ([209.85.128.194]:35257 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753646AbdDTLfo (ORCPT ); Thu, 20 Apr 2017 07:35:44 -0400 Received: by mail-wr0-f194.google.com with SMTP id l44so7128353wrc.2 for ; Thu, 20 Apr 2017 04:35:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=coEgGJ4mlSDSzJHg/cCaJak04nHubTgt0uTqFSMXyFg=; b=lzWGTeU0pmMf2F1EY9l/YEQbqA6/IWRjJ5WJsIpCbYDakWRo4ylNDolPXc9R9r/QIw S3mGwzcULtJrUEVhmH8461POjNLAzelFPv5RZSM286Y4qKosRXetVAfrFttcjQcfTMhj jdWcyfb0I46B0/DeJSHfeWzHAmJ2+ZYjX+VqC16be/VtdQq6YqOJnQcmxEAz7gd+6OOz n1s9eTJlSt7S2Nny+4iJj2EYbLpf/UBgX+w36UzL9YAr06GrYP26bjdW4+5hUpJyIpeZ a0qx2B7MHbUPthsx+O057FbDYsFKzxcnJth4N/oQLfbbeeMFpg8sP5WeVYdfqKSaL2Vo T+9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=coEgGJ4mlSDSzJHg/cCaJak04nHubTgt0uTqFSMXyFg=; b=cQYE/oFC3Mq8kAjGwWG6MmOYxyM1n2NWAuRs2ZWe1obuOUdojM5q0TztX1A7Uva8k5 yIhq+IXtBnFM/A35rhh0B+dR7ISpD8ZEZt8TKwYJLEzgyQc6qVknGVSi5jHBKACjYT1L YtXNTd/QHYpgcfo7531jOOG7RStYlVe1PRukRj+TULWqcycg+vRylKv+fdhxwVcsOU23 6GwUizUt7ocL1h0ZNrZhi4fUSeXgYOeUb5sP7kEHP0vZxakMya8Kn0V7POTbyRVnzRMs CHamSbgxMfdat+X6P7qzk16PPy961CPS6pwNJggkV4FZ9jWA4LEswWqje7ROc9PPhBvK nFcw== X-Gm-Message-State: AN3rC/6vugqUXrv2A4G58X6gqxR1JK6761+ERkWO7pu+597Wz6A4qgAe FfrgUWk15zrvfA== X-Received: by 10.223.150.35 with SMTP id b32mr7129565wra.78.1492688142550; Thu, 20 Apr 2017 04:35:42 -0700 (PDT) Received: from localhost (jirka.pirko.cz. [84.16.102.26]) by smtp.gmail.com with ESMTPSA id v52sm2809999wrc.53.2017.04.20.04.35.41 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 20 Apr 2017 04:35:41 -0700 (PDT) Date: Thu, 20 Apr 2017 13:35:40 +0200 From: Jiri Pirko To: Jamal Hadi Salim Cc: davem@davemloft.net, netdev@vger.kernel.org, eric.dumazet@gmail.com, xiyou.wangcong@gmail.com Subject: Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch Message-ID: <20170420113540.GC1886@nanopsycho.orion> References: <1492603050-9318-1-git-send-email-jhs@emojatatu.com> <20170419123645.GB3357@nanopsycho.orion> <705a4d67-4a81-113f-e22a-0ca0bb6cf1eb@mojatatu.com> <20170419131323.GE3357@nanopsycho.orion> <20170419155400.GH3357@nanopsycho.orion> <56488055-1f4b-ee10-2357-5ce80f1fbc50@mojatatu.com> <20170419161736.GJ3357@nanopsycho.orion> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Thu, Apr 20, 2017 at 12:42:55PM CEST, jhs@mojatatu.com wrote: >On 17-04-19 12:17 PM, Jiri Pirko wrote: >> Wed, Apr 19, 2017 at 06:07:48PM CEST, jhs@mojatatu.com wrote: >> > On 17-04-19 11:54 AM, Jiri Pirko wrote: >> > > Wed, Apr 19, 2017 at 05:37:15PM CEST, jhs@mojatatu.com wrote: >> > > > On 17-04-19 09:13 AM, Jiri Pirko wrote: >> > > > > Wed, Apr 19, 2017 at 03:03:59PM CEST, jhs@mojatatu.com wrote: >> > > > > > On 17-04-19 08:36 AM, Jiri Pirko wrote: >> > > > > > > Wed, Apr 19, 2017 at 01:57:29PM CEST, jhs@mojatatu.com wrote: >> > > > > > > > From: Jamal Hadi Salim >> > > > > > >> > > > > > > > include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++-- >> > > > > > > > net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++---------- >> > > > > > > > 3 files changed, 53 insertions(+), 12 deletions(-) >> > > > >> > > > > > > > +#define TCAA_MAX (__TCAA_MAX - 1) >> > > > > > > > #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg)))) >> > > > > > > > #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg)) >> > > > > > > > -#define TCA_ACT_TAB 1 /* attr type must be >=1 */ >> > > > > > > > -#define TCAA_MAX 1 >> > > > > > > > +#define TCA_ACT_TAB TCAA_ACT_TAB >> > > > > > > >> > > > > > > This is mess. What does "TCAA" stand for? >> > > > > > >> > > > > > TC Actions Attributes. What would you call it? I could have >> > > > > > called it TCA_ROOT etc. But maybe a comment to just call it >> > > > > > TC Actions Attributes would be enough? >> > > > > >> > > > > TCA_DUMP_X >> > > > > >> > > > > it is only for dumping. Naming it "attribute" seems weird. Same as if >> > > > > you have: int variable_something; >> > > > > >> > > > >> > > > Jiri, this is not just for dumping. We are describing high level >> > > > attributes for tc actions. >> > > >> > > This is already present: >> > > enum { >> > > TCA_ACT_UNSPEC, >> > > TCA_ACT_KIND, >> > > TCA_ACT_OPTIONS, >> > > TCA_ACT_INDEX, >> > > TCA_ACT_STATS, >> > > TCA_ACT_PAD, >> > > TCA_ACT_COOKIE, >> > > __TCA_ACT_MAX >> > > }; >> > > >> > > This is nested inside the dump message (TCAA_MAX, TCA_ACT_TAB) >> > > >> > > Looks like you are mixing these 2. >> > > >> > >> > No - That space is per action. The space i am defining is >> > above that in the the hierarchy. There used to be only >> > one attribute there (TCA_ACT_TAB) and now we are making >> > it more generic. >> >> Right. That's what I say. And that higher level is used only for >> dumping. That's why I suggested TCA_ACT_DUMP prefix. >> > >DUMP is not right. _TAB is used for SET/DEL as well. >why dont we leave this alone so we can progress? >You can submit changes later if it bothers you still. Ha. So the current code is wrong, I see it now. Following is needed: You confused me squashing the change to this patch. Ok, so the name could be: TCA_ACTS_* ? I believe it is crucial to figure this out right now. TC UAPI is in deep mess already, no need to push it even more. > >> >> > >> > > >> > > >> > >> > > > It is a _lot_ of code to change! Note: >> > > > This is all the UAPI visible code (the same coding style for 20 years). >> > > > I am worried about this part. >> > > >> > > We'll see. Lets do it in a sensitive way, in steps. But for new things, >> > > I think it is good not to stick with old and outlived habits. >> > > >> > > >> > >> > I know you have the muscle to get it done - so fine, i will start >> > with this one change. >> > >> > > >> > > Netlink is TLV, should be used as TLV. I don't see how you can run out >> > > any space. You tend to use Netlink in some weird hybrid mode, with only >> > > argument being space. I think that couple of bytes wasted is not >> > > a problem at all... >> > > >> > >> > You are not making sense to me still. >> > What you describe as "a couple of bytes" adds up when you have >> > a large number of objects. I am trying to cut down on data back >> > and forth from user/kernel and a bitmap is a well understood entity. >> >> The attributes in question are per-message, not per-object >> >> >> > >> > Even if i did use a TLV - when i am representing flags which require one >> > bit each - it makes sense to use a bitmask encapsulated in a TLV. Not >> > to waste a TLV per bit. >> >> That is the only correct way. For example, in future the kernel may >> report in extended ack that it does not support some specific attribute >> user passed. If you pack it all in one attr, that would not be possible. >> Also, what prevents user from passing random data on bit flag positions >> that you are not using? Current kernel would ignore it. Next kernel will >> do something different according to the flag bit. That is UAPI break. >> Essentialy the same thing what DaveM said about the struct. You have to >> define this completely, at the beginning, not possible to use the unused >> bits for other things in the future. >> > > >They are not the same issue Jiri. We have used bitmasks fine on netlink Howcome they are not the same? I'm pretty certain they are. >message for a millenia. Nobody sets garbage on a bitmask they are not >supposed to touch. The struct padding thing is a shame the way it >turned out - now netlink can no longer have a claim to be a (good) >wire protocol. >Other thing: I know you feel strongly about this but I dont agree that >everything has to be a TLV and in no way, as a matter of principle, you are >going to convince me (even when the cows come home) that I have to >use 64 bits to carry a single bit just so I can use TLVs. Then I guess we have to agree to disagree. I believe that your approach is wrong and will break users in future when you add even a single flag. Argument that "we are doing this for ages-therefore it is correct" has simply 0 value. diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 82b1d48..c432b22 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1005,7 +1005,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n, !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL, + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL, extack); if (ret < 0) return ret;