[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCHv2 2/3] grant_table: convert grant table rwlock to percpu rwlock
>>> On 25.11.15 at 14:43, <malcolm.crossley@xxxxxxxxxx> wrote: > On 25/11/15 12:35, Jan Beulich wrote: >>>>> On 20.11.15 at 17:03, <malcolm.crossley@xxxxxxxxxx> wrote: >>> @@ -208,8 +210,6 @@ active_entry_acquire(struct grant_table *t, grant_ref_t > e) >>> { >>> struct active_grant_entry *act; >>> >>> - ASSERT(rw_is_locked(&t->lock)); >> >> Even if not covering all cases, I don't think this should be dropped, >> just like you don't drop rw_is_write_locked() asserts. Would properly >> replacing this be rather expensive? > > I could add a percpu_rw_is_locked function but it will be racy because I > can't > set the local lock barrier variable ( set/clearing that would race with real > users of the write lock ) Therefore I would be left polling the percpu > readers > which is not reliable without the local barrier variable. Should I still add > the function if it won't reliably detect the locked condition? Iiuc that would then risk the ASSERT() producing false positives, which clearly is a no-go. In which case I'd still like you to leave in some sort of comment (perhaps just the current code converted to a comment) to document the requirement. >>> @@ -3180,7 +3178,7 @@ grant_table_create( >>> goto no_mem_0; >>> >>> /* Simple stuff. */ >>> - rwlock_init(&t->lock); >>> + percpu_rwlock_resource_init(&t->lock); >> >> Considering George's general comment (on patch 1), would it perhaps >> make sense to store (in debug builds) the per-CPU variable's offset >> in the lock structure, having percpu_{read,write}_lock() verify it? Or >> at the very least have gt_{read,write}_lock() wrappers, thus >> avoiding the need to explicitly pass grant_rwlock at each use site? I >> certainly agree with George that we should make it as hard as >> possible for someone to get using these constructs wrong. > > I can add storing the "per-CPU variable" owner in the local lock structure for > debug builds and have the percpu_{read,write} verify it at runtime. > > The downsides is that the size of structures will vary between debug and > non-debug > builds and that perhaps users may be confused what the per-CPU variables are > used for. I'm not sure what confusion might arise here. > I'll add the wrappers to for the grant table but will this make it harder to > determine how the locking works? Why would it? At the call sites we don't really care about the mechanism. But perhaps I'm not seeing what you're thinking of... Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |