Re: [patch 1/7] Oprofile AMD IBS support

[Maynard J. <may...@us...>]

Jason Yeh wrote:
> Maynard Johnson wrote:
> 
>>> +   --no-event       clear all events
>> Why is this new option *needed* for IBS support?
> The option was needed because at the time there is no
> way to clear all events saved in the daemon.rc file. If
> that's not the case any more, the option is not necessary.
As I'm sure you know, any 'opcontrol --event=<evt-name>' command will overwrite 
anything previously cahced in daemonrc file.  To my knowledge, there aren't any 
opcontrol options to clear any of the values cached by daemonrc.  My question to 
you is there a particular need for this with your IBS stuff?  Or was this just 
an add-on change that you thought was generally needed?  If the latter, please 
post as a separate patch.
> 
>>> +   --ibs-fetch=#count            enable AMD IBS Fetch sampling with 
>>> maximum count.
>>> +                                 Use 0 to disable IBS Fetch profiling.
>>> +                                 ( Default = 250000, Min = 25000, 
>>> Max = 1048575 )
>>> +
>>> +   --ibs-op=#count               enable AMD IBS OP sampling with 
>>> maximum count.
>>> +                                 Use 0 to disable IBS OP profiling.
>>> +                                 ( Default = 250000, Min = 25000, 
>>> Max = 1048575 )
>> Arch-specific options should be avoided.  Back in January when you 
>> posted version 1 of this patch set, you replied to a comment from John 
>> that one option would be to use '--event=ibs_[fetch | op]:<count>'.  I 
>> prefer that approach.
> ok. Those two options will rolled into --event in the next version.
> 
>>>                  fi
>>> +                if test `echo $val | grep IBS_FETCH`; then
>>> +                    echo "ERROR: $val is an IBS derived event."
>>> +                     echo "       Please specify --ibs-fetch instead."
>>> +                     echo "       See opcontrol --help for more 
>>> information."
>>> +                     exit 1
>>> +                 fi
>>> +    +                 if test `echo $val | grep IBS_OP`; then
>>> +                     echo "ERROR: $val is an IBS derived event."
>>> +                     echo "       Please specify --ibs-op instead."
>>> +                     echo "       See opcontrol --help for more 
>>> information."
>>> +                     exit 1
>>> +                 fi
>>  From the above, it appears you expect users to specify certain IBS 
>> fecth or op events via 'opcontrol --event=<evt-name>'.  I don't quite 
>> see the point.  Unless I'm missing something, this event name is never 
>> actually used for anything by either the daemon of the kernel driver.  
>> I suspect this was being done to avoid the automatic selection of the 
>> default event.  But if you make the change to use 'opcontrol 
>> --event=ibs_[fetch | op]:<count>' as mentioned above, then I think you 
>> could do away with this.
> The check is for making sure users are not specifying individual IBS 
> events.
> Users can only select either fetch or op, but not single events under the
> broad category.
Yes, I understand that.  But since you're going to switch to the 
'--event=ibs_[fetch | op]:count' method, is there any reason for a user to also 
specify one of your IBS_* events (e.g., '--event=IBS_FETCH_4K_PAGE')?  Your 
kernel driver code writes all appropriate info (fecth or ops) to the event 
buffer, and the user can pick and choose which "events" they want opreport to 
output by using the eventspec filter.  So, as I say above, I don't see the need 
for above code.  Am I missing something?

Thanks.
-Maynard
> 
>>> @@ -983,7 +1055,7 @@ do_kill_daemon()
>>>          fi
>>>  
>>>          COUNT=`expr $COUNT + 1`
>>> -        if test "$COUNT" -eq 15; then
>>> +        if test "$COUNT" -eq 60; then
>> Why is this change necessary?
> IBS generates mass amount of data needing. 15 seconds is often not
> long enough for the daemon to write out all the samples.
> 
>>> +    if test "$NR_CHOSEN" = 0 -a ! "$IBS_FETCH" != "0" -a ! "$IBS_OP" 
>>> != "0"; then
>> Hopefully, with the changes mentioned above, we can get rid of this.
> Yes. This will be redundant.
> 
>>>  
>>> +    if test "$IBS_FETCH" != 0; then
>>> +        OPD_ARGS="$OPD_ARGS --ibs-fetch=$IBS_FETCH"
>>> +    fi
>>> +
>>> +    if test "$IBS_OP" != 0; then
>>> +        OPD_ARGS="$OPD_ARGS --ibs-op=$IBS_OP"
>> These "--ibs-*" additions to OPD_ARGS don't seem to be necessary.  
>> There's nothing in your changes to the daemon that uses them.
> The daemon uses the "ibs-op" and "ibs-fetch" args to pass in the
> counter count. It will not be necessary if I made the change mentioned
> above.
> 
>>>  do_deinit()
>>>  {
>>>      # unmount /dev/oprofile if it is mounted
>>> -    OPROF_FS=`grep /dev/oprofile /etc/mtab`
>>> -    if test -n "$OPROF_FS"; then
>>> -        umount /dev/oprofile
>>> +    OPROF_FS_MOUNTED=`grep /dev/oprofile /etc/mtab`
>>> +    OPROF_FS=`grep /dev/oprofile /etc/mtab|cut --delimiter=" " 
>>> --fields=2`
>>> +    if test -n "$OPROF_FS_MOUNTED"; then
>>> +        umount $OPROF_FS
>> This change seems unrelated to your IBS stuff, right?  If so, please 
>> post a separate patch.  Either way, can you explain the need for it.
> The change is not related to IBS. It's for avoiding unmounting the
> hard coded path at /dev/oprofile and using the string from /etc/mtab.
> 
> Jason
>