From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jamal Hadi Salim Subject: Re: [PATCH v2 net-next] net:sched: add action inheritdsfield to skbedit Date: Mon, 28 May 2018 12:01:01 -0400 Message-ID: <17d6d359-8809-0fe7-9880-39fb24cf95ac@mojatatu.com> References: <20180528154228.GC3787@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "davem@davemloft.net" , "netdev@vger.kernel.org" , Michel Machado , "xiyou.wangcong@gmail.com" To: Marcelo Ricardo Leitner , "Fu, Qiaobin" Return-path: Received: from mail-qt0-f193.google.com ([209.85.216.193]:46950 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755076AbeE1QBE (ORCPT ); Mon, 28 May 2018 12:01:04 -0400 Received: by mail-qt0-f193.google.com with SMTP id h5-v6so7355255qtm.13 for ; Mon, 28 May 2018 09:01:04 -0700 (PDT) In-Reply-To: <20180528154228.GC3787@localhost.localdomain> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 28/05/18 11:42 AM, Marcelo Ricardo Leitner wrote: > On Mon, May 28, 2018 at 05:40:18AM +0000, Fu, Qiaobin wrote: >> The new action inheritdsfield copies the field DS of >> IPv4 and IPv6 packets into skb->priority. This enables >> later classification of packets based on the DS field. >> >> Original idea by Jamal Hadi Salim >> >> Signed-off-by: Qiaobin Fu >> Reviewed-by: Michel Machado >> --- >> >> Note that the motivation for this patch is found in the following discussion: >> https://www.spinics.net/lists/netdev/msg501061.html >> --- >> >> diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h >> index fbcfe27..432ad2f 100644 >> --- a/include/uapi/linux/tc_act/tc_skbedit.h >> +++ b/include/uapi/linux/tc_act/tc_skbedit.h >> @@ -30,9 +30,11 @@ >> #define SKBEDIT_F_MARK 0x4 >> #define SKBEDIT_F_PTYPE 0x8 >> #define SKBEDIT_F_MASK 0x10 >> +#define SKBEDIT_F_INHERITDSFIELD 0x20 >> >> struct tc_skbedit { >> tc_gen; >> + __u64 flags; > > I don't think this is doable. It looks like it was prepared for such > change, but it breaks UAPI as it causes tc without the respective > patch to not be able to talk to skbedit anymore: > > With this patch: > [root@f28 ~]# tc action add action skbedit priority 1 > RTNETLINK answers: Numerical result out of range > We have an error talking to the kernel > [root@f28 ~]# > > While without this patch: > [root@f28 ~]# tc action add action skbedit priority 1 > [root@f28 ~]# And the new flags field doesnt even seem to be in use. Qiaobin, you dont seem to need this. Also you have added a workaround for default_priority which I dont think is needed either (sounds like you want to make sure that SKBEDIT_F_PRIORITY and new feature are mutually exclusive - which seems unnecessary. My suggestion is to remove: + } else { +default_priority: + if (d->flags & SKBEDIT_F_PRIORITY) + skb->priority = d->priority; + } and leave alone: - if (d->flags & SKBEDIT_F_PRIORITY) - skb->priority = d->priority; If someone wants to set both flags, then let them. cheers, jamal