From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id C0A375A026F for ; Fri, 5 Jan 2024 11:28:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704450499; 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=L29mmxOBn5iSh1+fCa5vIxabmSK8MTW1izmbxorPVF8=; b=KbakeoEheeyA8gY0z+ok8bD2M/sTkZVlnylT+/0GSxbTLGZCKm0X2/wruiqxMLITG9ImVI Of7KBiN0k1ke8nR0PEskESU68vbv6Fx//I2JHGGZEberG2OvaIIghscAQGRCIx2Z6vaGtS ZTc9SYG2fI2X8XlRjgWvA8I+8DXZf9U= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-487-nF2o3qvEMAClkqxATKkl0g-1; Fri, 05 Jan 2024 05:28:18 -0500 X-MC-Unique: nF2o3qvEMAClkqxATKkl0g-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E9995101A52A; Fri, 5 Jan 2024 10:28:17 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 22577492BC6; Fri, 5 Jan 2024 10:28:16 +0000 (UTC) Date: Fri, 5 Jan 2024 11:27:54 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 13/13] flow: Avoid moving flow entries to compact table Message-ID: <20240105112754.75c765e3@elisabeth> In-Reply-To: References: <20231221061549.976358-1-david@gibson.dropbear.id.au> <20231221061549.976358-14-david@gibson.dropbear.id.au> <20231228192525.7ba1ee48@elisabeth> <20231230113304.37c60a9a@elisabeth> <20240102191341.7c91dd44@elisabeth> <20240105093335.0c725692@elisabeth> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: L3KSVQBDOU7Y5XWF56GFRFLPAIPI3FKV X-Message-ID-Hash: L3KSVQBDOU7Y5XWF56GFRFLPAIPI3FKV X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, 5 Jan 2024 20:39:50 +1100 David Gibson wrote: > On Fri, Jan 05, 2024 at 09:33:35AM +0100, Stefano Brivio wrote: > > On Thu, 4 Jan 2024 21:02:19 +1100 > > David Gibson wrote: > > > > > On Tue, Jan 02, 2024 at 07:13:41PM +0100, Stefano Brivio wrote: > > > > On Mon, 1 Jan 2024 23:01:17 +1100 > > > > David Gibson wrote: > > > > > > > > > On Sat, Dec 30, 2023 at 11:33:04AM +0100, Stefano Brivio wrote: > > > > > > On Thu, 28 Dec 2023 19:25:25 +0100 > > > > > > Stefano Brivio wrote: > > > > > > > > > > > > > > On Thu, 21 Dec 2023 17:15:49 +1100 > > > > > > > > David Gibson wrote: > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > I wonder if we really have to keep track of the number of (non-)entries > > > > > > > in the free "block", and if we have to be explicit about the two cases. > > > > > > > > > > > > > > I'm trying to find out if we can simplify the whole thing with slightly > > > > > > > different variants, for example: > > > > > > > > > > > > So... I think the version with (explicit) blocks has this fundamental > > > > > > advantage, on deletion: > > > > > > > > > > > > > > + flow->f.type = FLOW_TYPE_NONE; > > > > > > > > + /* Put it back in a length 1 free block, don't attempt to fully reverse > > > > > > > > + * flow_alloc()s steps. This will get folded together the next time > > > > > > > > + * flow_defer_handler runs anyway() */ > > > > > > > > + flow->free.n = 1; > > > > > > > > + flow->free.next = flow_first_free; > > > > > > > > + flow_first_free = FLOW_IDX(flow); > > > > > > > > > > > > which is doable even without explicit blocks, but much harder to > > > > > > follow. > > > > > > > > > > Remember this is not a general deletion, only a "cancel" of the most > > > > > recent allocation. > > > > > > > > Oh, I thought that was only the case for this series and you would use > > > > that as actual deletion in another pending series (which I haven't > > > > finished reviewing yet). > > > > > > No. Not allowing deletion of any entry at any time is what I'm > > > trading off to get both O(1) allocation and (effectively) O(1) > > > deletion. > > > > > > > But now I'm not sure anymore why I was thinking this... > > > > > > > > Anyway... do we really need it, then? Can't we just mark the "failed" > > > > flows as whatever means "closed" for a specific protocol, and clean > > > > them up later, instead of calling cancel() right away? > > > > > > We could, but I'm not sure we want to. For starters, that requires > > > protocol-specific behaviour whenever we need to back out an allocation > > > like this. Not a big deal, since that's in protocol specific code > > > already, but I think it's uglier than calling cancel. > > > > > > It also requires that the protocol specific deferred cleanup functions > > > (e.g. tcp_flow_defer()) handle partially initialised entries. With > > > 'cancel' we can back out just the initialisation steps we've already > > > done (because we know where we've failed during init), then remove the > > > entry. The deferred cleanup function only needs to deal with > > > "complete" entries. Again, certainly possible, but IMO uglier than > > > having 'cancel'. > > > > Okay, yes, I see now. > > > > Another doubt that comes to me now is: if you don't plan to use this > > alloc_cancel() thing anywhere else, the only reason why you are adding > > it is to replace the (flow_count >= FLOW_MAX) check with a flow_alloc() > > version that can fail. > > > > But at this point, speaking of ugliness, couldn't we just have a > > bool flow_may_alloc() { return flow_first_free < FLOW_MAX }; the caller > > can use to decide to abort earlier? To me it looks so much simpler and > > more robust. > > Well, we could, but there are a couple of reasons I don't love it. > The first is abstraction: this returns explicit handling of the layout > of the table to the protocol specific callers. It's not a huge deal > right now, but once we have 4 or 5 protocols doing this, having to > change all of them if we make any tiny change to the semantics of > flow_first_free isn't great. Hmm, I don't get the difference in terms of abstraction between checking the return value of flow_alloc() and checking the return value of flow_may_alloc(). In both cases the protocol handlers know that there's a table, a function to reserve entries, and that reserving entries might fail... and not much else. > The other issue is that to do this (without a bunch of fairly large > and ugly temporaries) means we'd populate at least some of the fields > in flow_common before we have officially "allocated" the entry. At > that point it becomes a bit fuzzy as to when that allocation really > occurs. Is it when we do the FLOW_MAX tesT? I would say yes -- after that we can't fail. I mean, we work with rather constrained structures for a number of reasons, which comes with a number of hard problems... let's at least reap the benefits of it? > Is it when we write to > f.type? Is it when we update flow_first_free? If we fail somewhere > in the middle of that, what steps do we need to reverse? We can't fail in the middle of it, at the moment. Of course, if the "allocation" function changes, we might need to change the scheme. But is it really likely? And anyway it's just a few lines in your current version... > For those reasons I prefer the scheme presented. Fwiw, in an earlier > draft I did this differently with a "flow_prealloc()", which was > essentially the check against FLOW_MAX, then a later > flow_alloc_commit(). I thought it turned out pretty confusing > compared to the alloc/cancel approach. The current flow_alloc_cancel() implementation is definitely simple and semantically clear. What worries me a bit is that you would have two different cases for free "blocks" of size one, depending on the order of the events. So if we want to debug something like that and we see a block with size one it might be a failed bind(), so a fake one, or also not: it might be an actual block with size one. Thinking of multithreading: defining flow_may_alloc() becomes more complicated because the caller can't just assume the "allocation" will succeed (as long as we don't cross an "epoll cycle" or something like that). But we'll probably need some form of locking or userspace RCU giving us barriers around the pair may_alloc() / alloc(). If we stick to the failing alloc(), this part is simpler, but the interpretation of flow_first_free and block sizes becomes non-trivial. Well, on the other hand, it's all simple enough that we can change it as needed (for example for multithreading). If we can hope that the new scheme is reasonably low on bugs and we'll probably never have to guess why a block has size one, I'm fine with the failing alloc() as well. -- Stefano