On Thu, Jan 21, 2021 at 09:50:34AM -0600, Eric W. Biederman wrote: > >> The current ucount code does check for overflow and fails the increment > >> in every case. > >> > >> So arguably it will be a regression and inferior error handling behavior > >> if the code switches to the ``better'' refcount_t data structure. > >> > >> I originally didn't use refcount_t because silently saturating and not > >> bothering to handle the error makes me uncomfortable. > >> > >> Not having to acquire the ucounts_lock every time seems nice. Perhaps > >> the path forward would be to start with stupid/correct code that always > >> takes the ucounts_lock for every increment of ucounts->count, that is > >> later replaced with something more optimal. > >> > >> Not impacting performance in the non-namespace cases and having good > >> performance in the other cases is a fundamental requirement of merging > >> code like this. > > > > Did I understand your suggestion correctly that you suggest to use > > spin_lock for atomic_read and atomic_inc ? > > > > If so, then we are already incrementing the counter under ucounts_lock. > > > > ... > > if (atomic_read(&ucounts->count) == INT_MAX) > > ucounts = NULL; > > else > > atomic_inc(&ucounts->count); > > spin_unlock_irq(&ucounts_lock); > > return ucounts; > > > > something like this ? > > Yes. But without atomics. Something a bit more like: > > ... > > if (ucounts->count == INT_MAX) > > ucounts = NULL; > > else > > ucounts->count++; > > spin_unlock_irq(&ucounts_lock); > > return ucounts; This is the original code. > I do believe at some point we will want to say using the spin_lock for > ucounts->count is cumbersome, and suboptimal and we want to change it to > get a better performing implementation. > > Just for getting the semantics correct we should be able to use just > ucounts_lock for locking. Then when everything is working we can > profile and optimize the code. > > I just don't want figuring out what is needed to get hung up over little > details that we can change later. OK. So I will drop this my change for now. -- Rgrds, legion
