Skip to contentSkip to conversation

- GRU Driver V3 fixes to resolve code review comments

PATCH In review Jack Steiner 2 participants 2 messages Jul 28, 2008 – Jul 29, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 - GRU Driver V3 fixes to resolve code review comments

Fixes problems identified in a code review:
- add comment with high level dscription of the GRU
- prepend "gru_" to all global names
- delete unused function
- couple of trivial bug fixes

---
 drivers/misc/sgi-gru/gru_instructions.h |   10 ----
 drivers/misc/sgi-gru/grufile.c          |    8 ++-
 drivers/misc/sgi-gru/grukservices.c     |    6 +-
 drivers/misc/sgi-gru/grumain.c          |   16 ++++--
 drivers/misc/sgi-gru/gruprocfs.c        |    4 -
 drivers/misc/sgi-gru/grutables.h        |   74 +++++++++++++++++++++++++++++---
 drivers/misc/sgi-gru/grutlbpurge.c      |    4 +
 7 files changed, 93 insertions(+), 29 deletions(-)
drivers/misc/sgi-gru/gru_instructions.h +0 -10
Index: linux/drivers/misc/sgi-gru/gru_instructions.h
===================================================================
@@ -285,16 +285,6 @@ __opword(unsigned char opcode, unsigned 
 }
 
 /*
- * Prefetch a cacheline. Fetch is unconditional. Must page fault if
- * no valid TLB entry is found.
- * 	??? should I use actual "load" or hardware prefetch???
- */
-static inline void gru_prefetch(void *p)
-{
-	*(volatile char *)p;
-}
-
-/*
  * Architecture specific intrinsics
  */
 static inline void gru_flush_cache(void *p)
drivers/misc/sgi-gru/grufile.c +6 -2
Index: linux/drivers/misc/sgi-gru/grufile.c
===================================================================
@@ -112,6 +112,10 @@ static int gru_file_mmap(struct file *fi
 	if ((vma->vm_flags & (VM_SHARED | VM_WRITE)) != (VM_SHARED | VM_WRITE))
 		return -EPERM;
 
+	if (vma->vm_start & (GRU_GSEG_PAGESIZE - 1) ||
+	    			vma->vm_end & (GRU_GSEG_PAGESIZE - 1))
+		return -EINVAL;
+
 	vma->vm_flags |=
 	    (VM_IO | VM_DONTCOPY | VM_LOCKED | VM_DONTEXPAND | VM_PFNMAP |
 			VM_RESERVED);
@@ -471,8 +475,8 @@ struct vm_operations_struct gru_vm_ops =
 module_init(gru_init);
 module_exit(gru_exit);
 
-module_param(options, ulong, 0644);
-MODULE_PARM_DESC(options, "Various debug options");
+module_param(gru_options, ulong, 0644);
+MODULE_PARM_DESC(gru_options, "Various debug options");
 
 MODULE_AUTHOR("Silicon Graphics, Inc.");
 MODULE_LICENSE("GPL");
drivers/misc/sgi-gru/grukservices.c +3 -3
Index: linux/drivers/misc/sgi-gru/grukservices.c
===================================================================
@@ -638,11 +638,11 @@ int gru_kservices_init(struct gru_state 
 	cpus_possible = uv_blade_nr_possible_cpus(gru->gs_blade_id);
 
 	num = GRU_NUM_KERNEL_CBR * cpus_possible;
-	cbr_map = reserve_gru_cb_resources(gru, GRU_CB_COUNT_TO_AU(num), NULL);
+	cbr_map = gru_reserve_cb_resources(gru, GRU_CB_COUNT_TO_AU(num), NULL);
 	gru->gs_reserved_cbrs += num;
 
 	num = GRU_NUM_KERNEL_DSR_BYTES * cpus_possible;
-	dsr_map = reserve_gru_ds_resources(gru, GRU_DS_BYTES_TO_AU(num), NULL);
+	dsr_map = gru_reserve_ds_resources(gru, GRU_DS_BYTES_TO_AU(num), NULL);
 	gru->gs_reserved_dsr_bytes += num;
 
 	gru->gs_active_contexts++;
@@ -673,7 +673,7 @@ int gru_kservices_init(struct gru_state 
 	}
 	unlock_cch_handle(cch);
 
-	if (options & GRU_QUICKLOOK)
+	if (gru_options & GRU_QUICKLOOK)
 		quicktest(gru);
 	return 0;
 }
drivers/misc/sgi-gru/grumain.c +10 -6
Index: linux/drivers/misc/sgi-gru/grumain.c
===================================================================
@@ -22,7 +22,7 @@
 #include "grutables.h"
 #include "gruhandles.h"
 
-unsigned long options __read_mostly;
+unsigned long gru_options __read_mostly;
 
 static struct device_driver gru_driver = {
 	.name = "gru"
@@ -163,14 +163,14 @@ static unsigned long reserve_resources(u
 	return bits;
 }
 
-unsigned long reserve_gru_cb_resources(struct gru_state *gru, int cbr_au_count,
+unsigned long gru_reserve_cb_resources(struct gru_state *gru, int cbr_au_count,
 				       char *cbmap)
 {
 	return reserve_resources(&gru->gs_cbr_map, cbr_au_count, GRU_CBR_AU,
 				 cbmap);
 }
 
-unsigned long reserve_gru_ds_resources(struct gru_state *gru, int dsr_au_count,
+unsigned long gru_reserve_ds_resources(struct gru_state *gru, int dsr_au_count,
 				       char *dsmap)
 {
 	return reserve_resources(&gru->gs_dsr_map, dsr_au_count, GRU_DSR_AU,
@@ -182,10 +182,10 @@ static void reserve_gru_resources(struct
 {
 	gru->gs_active_contexts++;
 	gts->ts_cbr_map =
-	    reserve_gru_cb_resources(gru, gts->ts_cbr_au_count,
+	    gru_reserve_cb_resources(gru, gts->ts_cbr_au_count,
 				     gts->ts_cbr_idx);
 	gts->ts_dsr_map =
-	    reserve_gru_ds_resources(gru, gts->ts_dsr_au_count, NULL);
+	    gru_reserve_ds_resources(gru, gts->ts_dsr_au_count, NULL);
 }
 
 static void free_gru_resources(struct gru_state *gru,
@@ -416,6 +416,7 @@ static void gru_free_gru_context(struct 
 
 /*
  * Prefetching cachelines help hardware performance.
+ * (Strictly a performance enhancement. Not functionally required).
  */
 static void prefetch_data(void *p, int num, int stride)
 {
@@ -746,6 +747,8 @@ again:
  * gru_nopage
  *
  * Map the user's GRU segment
+ *
+ * 	Note: gru segments alway mmaped on GRU_GSEG_PAGESIZE boundaries.
  */
 int gru_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
@@ -757,6 +760,7 @@ int gru_fault(struct vm_area_struct *vma
 		vma, vaddr, GSEG_BASE(vaddr));
 	STAT(nopfn);
 
+	/* The following check ensures vaddr is a valid address in the VMA */
 	gts = gru_find_thread_state(vma, TSID(vaddr, vma));
 	if (!gts)
 		return VM_FAULT_SIGBUS;
@@ -775,7 +779,7 @@ again:
 	}
 
 	if (!gts->ts_gru) {
-		while (!gru_assign_gru_context(gts)) {
+		if (!gru_assign_gru_context(gts)) {
 			mutex_unlock(&gts->ts_ctxlock);
 			preempt_enable();
 			schedule_timeout(GRU_ASSIGN_DELAY);  /* true hack ZZZ */
drivers/misc/sgi-gru/gruprocfs.c +2 -2
Index: linux/drivers/misc/sgi-gru/gruprocfs.c
===================================================================
@@ -122,7 +122,7 @@ static ssize_t statistics_write(struct f
 
 static int options_show(struct seq_file *s, void *p)
 {
-	seq_printf(s, "0x%lx\n", options);
+	seq_printf(s, "0x%lx\n", gru_options);
 	return 0;
 }
 
@@ -136,7 +136,7 @@ static ssize_t options_write(struct file
 	    (buf, userbuf, count < sizeof(buf) ? count : sizeof(buf)))
 		return -EFAULT;
 	if (!strict_strtoul(buf, 10, &val))
-		options = val;
+		gru_options = val;
 
 	return count;
 }
drivers/misc/sgi-gru/grutables.h +69 -5

104 lines not on this page · raw message

drivers/misc/sgi-gru/grutlbpurge.c +3 -1
Index: linux/drivers/misc/sgi-gru/grutlbpurge.c
===================================================================
@@ -242,7 +242,9 @@ static void gru_invalidate_range_end(str
 	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
 						 ms_notifier);
 
-	atomic_dec(&gms->ms_range_active);
+	/* ..._and_test() provides needed barrier */
+	(void)atomic_dec_and_test(&gms->ms_range_active);
+
 	wake_up_all(&gms->ms_wait_queue);
 	gru_dbg(grudev, "gms %p, start 0x%lx, end 0x%lx\n", gms, start, end);
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#1 lore raw
Quoted 6 lines · Jack Steiner

Index: linux/drivers/misc/sgi-gru/grutlbpurge.c
===================================================================
--- linux.orig/drivers/misc/sgi-gru/grutlbpurge.c 2008-07-28
14:52:26.000000000 -0500 +++
linux/drivers/misc/sgi-gru/grutlbpurge.c 2008-07-28 14:52:54.000000000
-0500 @@ -242,7 +242,9 @@ static void gru_invalidate_range_end(str

drivers/misc/sgi-gru/grutlbpurge.c static void gru_invalidate_range_end(str · line 248

Quoted 8 lines
drivers/misc/sgi-gru/grutlbpurge.c +3 -1
 	struct gru_mm_struct *gms = container_of(mn, struct gru_mm_struct,
 						 ms_notifier);
 
-	atomic_dec(&gms->ms_range_active);
+	/* ..._and_test() provides needed barrier */
+	(void)atomic_dec_and_test(&gms->ms_range_active);
+
 	wake_up_all(&gms->ms_wait_queue);

What about

if (atomic_dec_and_test())

    wake_up_all()

?

Also, I would prefer either a comment to say why the increment of ms_range_active is safe, or an smp_mb__after_atomic_inc() for it too.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>