On Tue, Oct 03, 2023 at 12:17:53AM +0200, Pablo Neira Ayuso wrote: > On Mon, Oct 02, 2023 at 11:50:25PM +0200, Pablo Neira Ayuso wrote: > > On Mon, Oct 02, 2023 at 08:06:42PM +0200, Phil Sutter wrote: > > > On Mon, Oct 02, 2023 at 11:05:16AM +0200, Pablo Neira Ayuso wrote: > > > > The dump and reset command should not refresh the timeout, this command > > > > is intended to allow users to list existing stateful objects and reset > > > > them, element expiration should be refresh via transaction instead with > > > > a specific command to achieve this, otherwise this is entering combo > > > > semantics that will be hard to be undone later (eg. a user asking to > > > > retrieve counters but _not_ requiring to refresh expiration). > > > > > > From a users' perspective, what is special about the element expires > > > value disqualifying it from being reset along with any counter/quota > > > values? > > > > > > Do you have a PoC for set element reset via transaction yet? Can we > > > integrate non-timeout resets with it, too? Because IIUC, that's an > > > alternative to the pending reset locking. > > > > Problem is listing is not supported from transaction path, this is > > using existing netlink dump infrastructure which runs lockless via > > rcu. So we could support reset, but we could not use netlink dump > > semantics to fetch the values, and user likely wants this to > > fetch-and-reset as in ctnetlink. > > Well, with NLM_F_ECHO, it should be possible to explore reset under > commit_mutex, but is it really worth the effort? I don't understand. Wasn't it your proposal to move things into the transaction? Above you write: "element expiration should be refresh via transaction instead". I asked what is special about timeout, why not handle all element state reset the same way? > With two concurrent threads, we just want to ensure that no invalid > state shows in the listing (you mentioned it is possible to list > negative values with two threads listing-and-resetting at the same > time). It's not just in the listing, the actual values underrun. A quota e.g. will immediately deny. > I think we should just make sure something valid is included in the > listing, but as for the two threads performing list-and-reset, why > ensure strict serialization? I seem to lack context here. Is there an alternative to "strict" serializing? Expressions' dump+reset callbacks must not run multiple times at the same time for the same expression. At least not how they are currently implemented. > This is a rare operation to fetch statistics, most likely having a > single process performing this in place? So we are currently > discussing how to fix the (negative) non-sense in the listing. > > > > What we have now is a broad 'reset element', not specifying what to > > > reset. If the above is a feature being asked for, I'd rather implement > > > 'reset element counter', 'reset element timeout', 'reset element quota', > > > etc. commands. > > > > We are currently discussing how to implement refresh timeout into the > > transaction model. > > > > I would suggest we keep this chunk away by now for the _RESET command, > > until we agree on next steps. I would suggest to leave things as-is until there's hard evidence why it has to change now or there is a viable alternative implementation. It's your call to make since you're the maintainer of the project, but please stick to the standards you demand from other contributors. Cheers, Phil
