From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id CD3A43B29E for ; Fri, 26 Jun 2020 08:52:40 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593175960; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=hD2c3oEpNKtEub0ENNXKHCIg/QZcL62Hd+GpAUC1+6s=; b=K4VHne8TUFZamSCLFy4xCs07lzhZda6sSojeNpuRN+xnl2ACeuaU2c8gJcgCzliRb6pSBK xsYDP1LY3kpKYvAlErWwNCSxJsdIZausdRDuk+Shzw6estT3R8elGpy6CRRu20rI4HfJrp eqYWQXhZ/V+KifsBC6sQjnlQayeiDzI= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-47-yNxuKqexO1eV0zYM9mNZKw-1; Fri, 26 Jun 2020 08:52:38 -0400 X-MC-Unique: yNxuKqexO1eV0zYM9mNZKw-1 Received: by mail-ej1-f69.google.com with SMTP id cf15so7077474ejb.6 for ; Fri, 26 Jun 2020 05:52:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version:content-transfer-encoding; bh=/iHDgfiY8TDh3EPNTgzupA4JBUtTDmY+m7kSnSwNTKo=; b=nS827Q0EMfYPIU4oOaLv9Yd3rgn3iipzjuiHlKk/IDoMuyDynUx/DcLPyVxsk+dRdJ lhLJK4GB6Sn9LB3OypFkyDkW6xQeBMkj3jM+3drRguQgk+brTGl/94Qg4T/4i7po+AdB kf3XCFT5F6k5Y/HTni/SFRlA4EFR6LhRuisWIa4RzuELZDjqGEy7UOSuC4MwjZqALMAt x/pyaQ8qmy8x/eXZA6i15sIL3xsP3h7UXfdLibGpWWRvCcmo0SHDcjgsghIcg+bHq4HT QrZI53PdvdtycF++aIhnErdRE7noVbrBtkLLxW7LLPkd3jOT57JMYmFyo7yXFr8wCAc7 iZKw== X-Gm-Message-State: AOAM530FJAatpe8e4X5O0FQB2T/4PWMMMF/ZNdgec/r6eCuPzZsN/PmA VNfbqtmzP2wcLShI9++hARd2nt+xGFqcbvpRD7/wrxC1k7hDFrJlWCYBzkWrh6GOgKfaDl87BeY eI4WmZeZbjoMaZup7/B80Tg== X-Received: by 2002:a17:906:6a4f:: with SMTP id n15mr2350261ejs.378.1593175957584; Fri, 26 Jun 2020 05:52:37 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz3UPQSYD18e7LtAU1xQDYajPQGgJdas4L4X0+hY7qoM0GoqUwJwTAhxcLFtToqZitlTRnD7w== X-Received: by 2002:a17:906:6a4f:: with SMTP id n15mr2350245ejs.378.1593175957307; Fri, 26 Jun 2020 05:52:37 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk ([2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id o17sm10814176ejb.105.2020.06.26.05.52.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Jun 2020 05:52:36 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 116EC1808CF; Fri, 26 Jun 2020 14:52:36 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Davide Caratti , David Miller Cc: cake@lists.bufferbloat.net, netdev@vger.kernel.org, Jiri Pirko , Jamal Hadi Salim In-Reply-To: <240fc14da96a6212a98dd9ef43b4777a9f28f250.camel@redhat.com> References: <159308610282.190211.9431406149182757758.stgit@toke.dk> <159308610390.190211.17831843954243284203.stgit@toke.dk> <20200625.122945.321093402617646704.davem@davemloft.net> <87k0zuj50u.fsf@toke.dk> <240fc14da96a6212a98dd9ef43b4777a9f28f250.camel@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett Date: Fri, 26 Jun 2020 14:52:36 +0200 Message-ID: <87h7uyhtuz.fsf@toke.dk> MIME-Version: 1.0 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=toke@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Cake] [PATCH net-next 1/5] sch_cake: fix IP protocol handling in the presence of VLAN tags X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Jun 2020 12:52:40 -0000 Davide Caratti writes: > hello, > > my 2 cents: > > On Thu, 2020-06-25 at 21:53 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote= : >> I think it depends a little on the use case; some callers actually care >> about the VLAN tags themselves and handle that specially (e.g., >> act_csum). > > I remember taht something similar was discussed about 1 year ago [1]. Ah, thank you for the pointer! >> Whereas others (e.g., sch_dsmark) probably will have the same >> issue. > > I'd say that the issue "propagates" to all qdiscs that mangle the ECN-CE > bit (i.e., calling INET_ECN_set_ce() [2]), most notably all the RED > variants and "codel/fq_codel". Yeah, I think we should fix INET_ECN_set_ce() instead of re-implementing it in CAKE. See below, though. >> I guess I can trying going through them all and figuring out if >> there's a more generic solution. > > For sch_cake, I think that the qdisc shouldn't look at the IP header when > it schedules packets having a VLAN tag. > > Probably, when tc_skb_protocol() returns ETH_P_8021Q or ETH_P_8021AD, we > should look at the VLAN priority (PCP) bits (and that's something that > cake doesn't do currently - but I have a small patch in my stash that > implements this: please let me know if you are interested in seeing it :) > ). > > Then, to ensure that the IP precedence is respected, even with different > VLAN tags, users should explicitly configure TC filters that "map" the > DSCP value to a PCP value. This would ensure that configured priority is > respected by the scheduler, and would also be flexible enough to allow > different "mappings". I think you have this the wrong way around :) I.e., classifying based on VLAN priority is even more esoteric than using diffserv markings, so that should not be the default. Making it the default would also make the behaviour change for the same traffic if there's a VLAN tag present, which is bound to confuse people. I suppose it could be an option, but not really sure that's needed, since as you say you could just implement it with your own TC filters... > Sure, my proposal does not cover the problem of mangling the CE bit > inside VLAN-tagged packets, i.e. if we should understand if qdiscs > should allow it or not. Hmm, yeah, that's the rub, isn't it? I think this is related to this commit, which first introduced tc_skb_protocol(): d8b9605d2697 ("net: sched: fix skb->protocol use in case of accelerated vla= n path") That commit at least made the behaviour consistent across accelerated/non-accelerated VLANs. However, the patch description asserts that 'tc code .. expects vlan protocol type [in skb->protocol]'. Looking at the various callers, I'm not actually sure that's true, in the sense that most of the callers don't handle VLAN ethertypes at all, but expects to find an IP header. This is the case for at least: - act_ctinfo - act_skbedit - cls_flow - em_ipset - em_ipt - sch_cake - sch_dsmark In fact the only caller that explicitly handles a VLAN ethertype seems to be act_csum; and that handles it in a way that also just skips the VLAN headers, albeit by skb_pull()'ing the header. cls_api, em_meta and sch_teql don't explicitly handle it; but returning the VLAN ethertypes to those does seem to make sense, since they just pass the value somewhere else. So my suggestion would be to add a new helper that skips the VLAN tags and finds the L3 ethertype (i.e., basically cake_skb_proto() as introduced in this patch), then switch all the callers listed above, as well as the INET_ECN_set_ce() over to using that. Maybe something like 'skb_l3_protocol()' which could go into skbuff.h itself, so the ECN code can also find it? Any objections to this? It's not actually clear to me how the discussion you quoted above landed; but this will at least make things consistent across all the different actions/etc. Adding in Jiri and Jamal as well since they were involved in the patch I quoted above. -Toke