search for: mmu_range_set_seq

...+ struct mm_struct *mm); > +int mmu_range_notifier_insert_locked(struct mmu_range_notifier *mrn, > + unsigned long start, unsigned long length, > + struct mm_struct *mm); > +void mmu_range_notifier_remove(struct mmu_range_notifier *mrn); > + > +/** > + * mmu_range_set_seq - Save the invalidation sequence How about: * mmu_range_set_seq - Set the .invalidate_seq to a new value. > + * @mrn - The mrn passed to invalidate > + * @cur_seq - The cur_seq passed to invalidate > + * > + * This must be called unconditionally from the invalidate callback of a &g...

...> ...other ideas? This odd duality has already cause some confusion, but names here are hard. mmu_interval_notifier is the best alternative I've heard. Changing this name is a lot of work - are we happy 'mmu_interval_notifier' is the right choice? > > +/** > > + * mmu_range_set_seq - Save the invalidation sequence > > How about: > > * mmu_range_set_seq - Set the .invalidate_seq to a new value. It is not a 'new value', it is a value that is provided to the invalidate callback > > > + * @mrn - The mrn passed to invalidate > > + * @cur_se...

...ops.invalidate() i failed to catch > that in v1 sorry. I think what you mean is already done? struct mmu_range_notifier_ops { bool (*invalidate)(struct mmu_range_notifier *mrn, const struct mmu_notifier_range *range, unsigned long cur_seq); > No it is always odd, you must call mmu_range_set_seq() only from the > op->invalidate_range() callback at which point the seq is odd. As well > when mrn is added and its seq first set it is set to an odd value > always. Maybe the comment, should read: > > * mrn->invalidate_seq is always, yes always, set to an odd value. This en...

...at file closely there is more. :) > > Lets change that to rtnl_unlock() then Thanks :) ... >>> + * mrn->invalidate_seq is always set to an odd value. This ensures >> >> This claim just looks wrong the first N times one reads the code, given that >> there is mmu_range_set_seq() to set it to an arbitrary value! Maybe >> you mean > > mmu_range_set_seq() is NOT to be used to set to an arbitary value, it > must only be used to set to the value provided in the invalidate() > callback and that value is always odd. Lets make this super clear: > > /*...

..._mm->invalidate_seq > > + * spin_unlock > > + * spin_lock > > + * seq = ++mmn_mm->invalidate_seq > > + * spin_unlock > > + * op->invalidate_range(): > > + * user_lock > > + * mmu_range_set_seq() > > + * mrn->invalidate_seq = seq > > + * user_unlock > > + * > > + * [Required: mmu_range_read_retry() == true] > > + * > > + * mn_itree_inv_end(): > > + * spin_lock > > + * seq = ++mmn_mm-...

...67,21 +67,15 @@ static bool amdgpu_mn_invalidate_gfx(struct mmu_range_notifier *mrn, > struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > long r; > > - /* > - * FIXME: Must hold some lock shared with > - * amdgpu_ttm_tt_get_user_pages_done() > - */ > - mmu_range_set_seq(mrn, cur_seq); > + mutex_lock(&adev->notifier_lock); > > - /* FIXME: Is this necessary? */ > - if (!amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm, range->start, > - range->end)) > - return true; > + mmu_range_set_seq(mrn, cur_seq); > > - if (!mmu_not...

...long start, unsigned long length, + struct mm_struct *mm); +int mmu_range_notifier_insert_locked(struct mmu_range_notifier *mrn, + unsigned long start, unsigned long length, + struct mm_struct *mm); +void mmu_range_notifier_remove(struct mmu_range_notifier *mrn); + +/** + * mmu_range_set_seq - Save the invalidation sequence + * @mrn - The mrn passed to invalidate + * @cur_seq - The cur_seq passed to invalidate + * + * This must be called unconditionally from the invalidate callback of a + * struct mmu_range_notifier_ops under the same lock that is used to call + * mmu_range_read_retry(...

...alidate)(struct mmu_range_notifier *mrn, > const struct mmu_notifier_range *range, > unsigned long cur_seq); Yes it is sorry, i got confuse with mmu_range_notifier and mmu_notifier_range :) It is almost a palyndrome structure ;) > > > No it is always odd, you must call mmu_range_set_seq() only from the > > op->invalidate_range() callback at which point the seq is odd. As well > > when mrn is added and its seq first set it is set to an odd value > > always. Maybe the comment, should read: > > > > * mrn->invalidate_seq is always, yes always, set...

On Tue, Oct 29, 2019 at 07:22:37PM +0000, Yang, Philip wrote: > Hi Jason, > > I did quick test after merging amd-staging-drm-next with the > mmu_notifier branch, which includes this set changes. The test result > has different failures, app stuck intermittently, GUI no display etc. I > am understanding the changes and will try to figure out the cause. Thanks! I'm not

...mn_invalidate_gfx(struct mmu_range_notifier *mrn, >> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >> long r; >> >> - /* >> - * FIXME: Must hold some lock shared with >> - * amdgpu_ttm_tt_get_user_pages_done() >> - */ >> - mmu_range_set_seq(mrn, cur_seq); >> + mutex_lock(&adev->notifier_lock); >> >> - /* FIXME: Is this necessary? */ >> - if (!amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm, range->start, >> - range->end)) >> - return true; >> + mmu_range_set_seq(mrn, cur_seq...

...{ > struct amdgpu_bo *bo = container_of(mrn, struct amdgpu_bo, notifier); > struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > long r; > > + /* > + * FIXME: Must hold some lock shared with > + * amdgpu_ttm_tt_get_user_pages_done() > + */ > + mmu_range_set_seq(mrn, cur_seq); > + > /* FIXME: Is this necessary? */ > if (!amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm, range->start, > range->end)) > @@ -119,11 +104,18 @@ static const struct mmu_range_notifier_ops amdgpu_mn_gfx_ops = { > * evicting all user-mode queues...

...er_range *range, + unsigned long cur_seq) { struct amdgpu_bo *bo = container_of(mrn, struct amdgpu_bo, notifier); struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); long r; + /* + * FIXME: Must hold some lock shared with + * amdgpu_ttm_tt_get_user_pages_done() + */ + mmu_range_set_seq(mrn, cur_seq); + /* FIXME: Is this necessary? */ if (!amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm, range->start, range->end)) @@ -119,11 +104,18 @@ static const struct mmu_range_notifier_ops amdgpu_mn_gfx_ops = { * evicting all user-mode queues of the process. */ static bool...

...ree > - scif_dma has a complicated logic flow > - vhost's mmu notifiers are already being rewritten > > This series, and the other code it depends on is available on my github: > > https://github.com/jgunthorpe/linux/commits/mmu_notifier > > v2 changes: > - Add mmu_range_set_seq() to set the mrn sequence number under the driver > lock and make the locking more understandable > - Add some additional comments around locking/READ_ONCe > - Make the WARN_ON flow in mn_itree_invalidate a bit easier to follow > - Fix wrong WARN_ON > > Jason Gunthorpe (15): &...

...tudy - hfi1 (2nd user) needs access to the rbtree - scif_dma has a complicated logic flow - vhost's mmu notifiers are already being rewritten This series, and the other code it depends on is available on my github: https://github.com/jgunthorpe/linux/commits/mmu_notifier v2 changes: - Add mmu_range_set_seq() to set the mrn sequence number under the driver lock and make the locking more understandable - Add some additional comments around locking/READ_ONCe - Make the WARN_ON flow in mn_itree_invalidate a bit easier to follow - Fix wrong WARN_ON Jason Gunthorpe (15): mm/mmu_notifier: define the he...