Re: [PATCH v6 8/8] nouveau/svm: Implement atomic SVM access

On Monday, 15 March 2021 6:51:13 PM AEDT Christoph Hellwig wrote:

...

• /*XXX: atomic? */
• return (fa->access == 0 || fa->access == 3) -

•       (fb->access == 0 || fb->access == 3);
  

• /* Atomic access (2) has highest priority */
• return (-1*(fa->access == 2) + (fa->access == 0 || fa->access == 3)) -

•       (-1*(fb->access == 2) + (fb->access == 0 || fb->access == 3));
  

This looks really unreabable. If the magic values 0, 2 and 3 had names it might become a little more understadable, then factor the duplicated calculation of the priority value into a helper and we'll have code that mere humans can understand..

Fair enough, will add some definitions for the magic values.

...

• mutex_lock(&svmm->mutex);
  

• if (mmu_interval_read_retry(&notifier->notifier,
  

• 			    notifier_seq)) {
  

• 	mutex_unlock(&svmm->mutex);
  

• 	continue;
  

• }
  

• break;
  

• }

This looks good, why not:

mutex_lock(&svmm->mutex);
if (!mmu_interval_read_retry(&notifier->notifier,
			     notifier_seq))
	break;
mutex_unlock(&svmm->mutex);

}

I had copied that from nouveau_range_fault() but this suggestion is better. Will update, thanks for looking.