On Thu, 4 Jan 2024 21:02:19 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Tue, Jan 02, 2024 at 07:13:41PM +0100, Stefano Brivio wrote: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. -- StefanoOn Mon, 1 Jan 2024 23:01:17 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: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.On Sat, Dec 30, 2023 at 11:33:04AM +0100, Stefano Brivio wrote: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).On Thu, 28 Dec 2023 19:25:25 +0100 Stefano Brivio <sbrivio(a)redhat.com> wrote: > > On Thu, 21 Dec 2023 17:15:49 +1100 > > David Gibson <david(a)gibson.dropbear.id.au> 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.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'.