From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DCCE619BDF for ; Tue, 11 Jul 2023 20:47:15 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 245DB1987 for ; Tue, 11 Jul 2023 13:47:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689108419; 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: in-reply-to:in-reply-to:references:references; bh=s4WnwWEWs/PWwc+6zWRXmlvXFfzoTik2Yf9g1/Rp5QY=; b=D4XcY5L6qhEDVA2r84UcoMCDZhod9JQRIM7VqkkR/ce7UDjBMECWX84ySD1Wi4K8jetj9m l0vw6woWElBkZ9RMV6KD/SdXsggBR0G8jeDXs4iXkWHi05S3Jj42JxIIBqJ6QDUWse3hqt M53ZKhrxqBNTBzxRRsoMCLe7cFuDX+s= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-562-cQ5nioz9OICDcD51Jcz3-A-1; Tue, 11 Jul 2023 16:46:58 -0400 X-MC-Unique: cQ5nioz9OICDcD51Jcz3-A-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0807D3C1D603; Tue, 11 Jul 2023 20:46:58 +0000 (UTC) Received: from RHTPC1VM0NT (unknown [10.22.34.58]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1A367200AD6E; Tue, 11 Jul 2023 20:46:57 +0000 (UTC) From: Aaron Conole To: Eric Garver Cc: Ilya Maximets , Jakub Kicinski , netdev@vger.kernel.org, dev@openvswitch.org, Paolo Abeni , Eric Dumazet , "David S. Miller" , Adrian Moreno , Eelco Chaudron Subject: Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action References: <20230629203005.2137107-1-eric@garver.life> <20230629203005.2137107-3-eric@garver.life> <6060b37e-579a-76cb-b853-023cb1a25861@ovn.org> <20230707080025.7739e499@kernel.org> <20230707150610.4e6e1a4d@kernel.org> <096871e8-3c0b-d5d7-8e68-833ba26b3882@ovn.org> Date: Tue, 11 Jul 2023 16:46:56 -0400 In-Reply-To: (Eric Garver's message of "Mon, 10 Jul 2023 14:21:24 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Eric Garver writes: > On Mon, Jul 10, 2023 at 06:51:19PM +0200, Ilya Maximets wrote: >> On 7/8/23 00:06, Jakub Kicinski wrote: >> > On Fri, 7 Jul 2023 18:04:36 +0200 Ilya Maximets wrote: >> >>>> That already exists, right? Johannes added it in the last release for WiFi. >> >>> >> >>> I'm not sure. The SKB_DROP_REASON_SUBSYS_MAC80211_UNUSABLE behaves similarly >> >>> to that on a surface. However, looking closer, any value that can be passed >> >>> into ieee80211_rx_handlers_result() and ends up in the kfree_skb_reason() is >> >>> kind of defined in net/mac80211/drop.h, unless I'm missing something (very >> >>> possible, because I don't really know wifi code). >> >>> >> >>> The difference, I guess, is that for openvswitch values will be provided by >> >>> the userpsace application via netlink interface. It'll be just a number not >> >>> defined anywhere in the kernel. Only the subsystem itself will be defined >> >>> in order to occupy the range. Garbage in, same garbage out, from the kernel's >> >>> perspective. >> >> >> >> To be clear, I think, not defining them in this particular case is better. >> >> Definition of every reason that userspace can come up with will add extra >> >> uAPI maintenance cost/issues with no practical benefits. Values are not >> >> going to be used for anything outside reporting a drop reason and subsystem >> >> offset is not part of uAPI anyway. >> > >> > Ah, I see. No, please don't stuff user space defined values into >> > the drop reason. The reasons are for debugging the kernel stack >> > itself. IOW it'd be abuse not reuse. >> >> Makes sense. I wasn't sure that's a good solution from a kernel perspective >> either. It's better than defining all these reasons, IMO, but it's not good >> enough to be considered acceptable, I agree. >> >> How about we define just 2 reasons, e.g. OVS_DROP_REASON_EXPLICIT_ACTION and >> OVS_DROP_REASON_EXPLICIT_ACTION_WITH_ERROR (exact names can be different) ? >> One for an explicit drop action with a zero argument and one for an explicit >> drop with non-zero argument. >> >> The exact reason for the error can be retrieved by other means, i.e by looking >> at the datapath flow dump or OVS logs/traces. >> >> This way we can give a user who is catching packet drop traces a signal that >> there was something wrong with an OVS flow and they can look up exact details >> from the userspace / flow dump. >> >> The point being, most of the flows will have a zero as a drop action argument, >> i.e. a regular explicit packet drop. It will be hard to figure out which flow >> exactly we're hitting without looking at the full flow dump. And if the value >> is non-zero, then it should be immediately obvious which flow is to blame from >> the dump, as we should not have a lot of such flows. >> >> This would still allow us to avoid a maintenance burden of defining every case, >> which are fairly meaningless for the kernel itself, while having 99% of the >> information we may need. >> >> Jakub, do you think this will be acceptable? >> >> Eric, Adrian, Aaron, do you see any problems with such implementation? > > I see no problems. I'm content with this approach. +1 >> P.S. There is a plan to add more drop reasons for other places in openvswitch >> module to catch more regular types of drops like memory issues or upcall >> failures. So, the drop reason subsystem can be extended later. >> The explicit drop action is a bit of an odd case here. >> >> Best regards, Ilya Maximets. >>