Re: [PATCH] PowerPC: Add Program Priority Register support

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

Thanks for the review Ryan, comments below:

On 08/14/2012 12:53 PM, Ryan S. Arnold wrote:

> On Tue, Aug 14, 2012 at 8:03 AM, Adhemerval Zanella
> <azanella@linux.vnet.ibm.com> wrote:
>> +The @dfn{Program Prioriry Register} (PPR) is a 64-bit register that controls
> Typo "Prioriry" -> "Priority"

Fixed.

>> +the program's priority. By adjusting the PPR value the programmer may
>> +improve the system throughput by causing the system resouces to be used
> "improve the system" -> "improve system"
>
> "the resouces" -> "the resources"

Fixed.

>> +more efficiently, especially in contention situations. More information
>> +available in @cite{Power ISA 2.06b - Book II - Section 3.1}.
>> +@end deftypefun
> Perhaps a small indication of what states are available to user state
> programs vs. privileged states would be useful here, similar to what
> you have in ppc.h?
>
> Also, can you describe what the current default priority is and what
> it will be per ISA 2.06b when the kernel has complied?

I changed to:

 @deftypefun {void} __ppc_set_ppr_med (void)
-Set the Program Priority Register to medium value.
+Set the Program Priority Register to medium value (default).
 
 The @dfn{Program Priority Register} (PPR) is a 64-bit register that controls
 the program's priority. By adjusting the PPR value the programmer may
 improve system throughput by causing the system resources to be used
-more efficiently, especially in contention situations. More information
+more efficiently, especially in contention situations.
+The three unprivileged states available are covered by the functions
+__ppc_set_ppr_med (medium - default), __ppc_set_ppc_low (low) and
+__ppc_set_ppc_med_low (medium low).  More information
 available in @cite{Power ISA 2.06b - Book II - Section 3.1}.

I think privileged states should not be take in details in this documentation,
since they are not accessible in userspace.

>> +/* The Priority Program Register (PPR) is saved and restored by moving its
>> +   value to ppr field in TBC. Since it is the loader responsible to setup the
> "TBC" -> "TCB"
>
> "loader responsible" -> "loader's responsibility"

Fixed.

>> +   TBC, the code is only built if not within the loader.  */
> "TBC" -> "TCB"

Fixed.

> "not within the loader." -> "not being used within the loader."

Fixed.

>> +/* The Priority Program Register (PPR) is saved and restored by moving its
>> +   value to ppr field in TBC. Since it is the loader responsible to setup the
>> +   TBC, the code is only built if not within the loader.  */
> Same comments as in the powerpc32 version above.

Fixed.

>> +  SET_PPR                      \
>> +  li r0,syscall;               \
>> +  sc;                          \
>> +  RESTORE_PPR
>>
> When used in the context of register state preservation rules isn't
> the terminology usually SAVE/RESTORE or SET/GET?  Such as the case in
> the _context functions?

Changed to SAVE_PPR/RESTORE_PPR.

>> +
>> +/*
>> + * ISA 2.05 and beyond support the Program Priority Register (PPR) to adjust
>> + * thread priorities based on lock acquisition, wait and release. The ISA
>> + * defines the use of form 'or Rx,Rx,Rx' as the way to modify the PRI field.
>> + * The unprivileged priorities are:
>> + *   Rx = 1 (low)
>> + *   Rx = 2 (medium)
>> + *   Rx = 6 (medium-low/normal)
>> + * The 'or' instruction form is a nop in previous hardware, so it is safe to
>> + * use unguarded.
>> + */
> Please indicate what the 'default' priority is now (kernel version)
> and what it is intended to be in the future.

Added:

- * use unguarded.
+ * use unguarded. The default value is 'medium'.

-- 
Adhemerval Zanella Netto
  Software Engineer
  Linux Technology Center Brazil
  Toolchain / GLIBC on Power Architecture
  azanella@linux.vnet.ibm.com / azanella@br.ibm.com
  +55 61 8642-9890