Re: [PATCH 2/2] S/390: Adjust the user space tools for System z hardware sampling

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

Andreas Krebbel wrote:
> On 12/07/2011 12:14 AM, Maynard Johnson wrote:
>> On 12/05/2011 8:55 AM, Andreas Krebbel wrote:
>>> The attached patch implements the following adjustments to the
>>> OProfile user space parts:
>> Andreas,
>> A few minor comments below . . .
> 
> Thanks for the quick review!
> 
>>> +	if (cur>  1&&  timer_event_found_p) {
>>> +		fprintf(stderr, "oprofiled: " TIMER_EVENT_NAME " cannot be used"
>>> +			" in combination with hardware counters.\n");
>>> +		exit(EXIT_FAILURE);
>>> +	}
> 
> Ok. I'll put a similiar check into opcontrol but will leave the sanity check in here as
> well. Just in case the daemon is started manually. Ok?

I saw your other note where you said the validation is already done via verify_counters, so that's good.  But please remove this from code from daemon/opd_events.c since 1) it's redundant; 2) why should we just do this validation and not others; and 3) starting oprofiled manually is not recommended.

> 
>>> + 				descr->name = "HWSAMPLING";
>>> + 				descr->count = 4127518;
>>                                                 ^-- why does your events file 
>> have 2202 for a minimum?  That's a long way from 4127518!
> 
> 2202 is by far too low for any real use. Taking a sample every 4127518 (4M) cycles on a
> 5.2 GHz machine like z196 gives you about 1260 samples per second. This turned out to be a
> plausible value to start with. That's the same default as chosen in the kernel module.
> Dunno why the kernel guys did chose a power of two here there is actually no reason for that.
> 
>>> + 			} else {
>>> + 				descr->name = TIMER_EVENT_NAME;
>>> + 				descr->count = 10000;
>>
>> Isn't the timer-mode setting a frequency, not a count?  I don't have a big 
>> problem re-using the count field to hold the frequency, but perhaps we need 
>> something added to the documentation to explain this, since this little wart is 
>> exposed when the user passes '--event=TIMER:<count>'.
> 
> Yeah the count value for the timer event is absolutely meaningless here and is currently
> ignored anyway. This is just for the follow-on work proposed by Robert to make this value
> configurable and I wanted to prepare s390 for that :)
OK, I lost track of where that discussion thread was.
> 
> I don't know how 'count' should be defined for the timer event. It would be desirable to
> make this a real count as it is with every other counter. On the other hand the timer
> interrupt occurrences in general cannot freely be chosen. Otherwise the kernel module
> could use the clock rate and the count value to calculate how the timer interrupt must be
> configured. But this belongs into the follow-on work part I think.
So the count value will be ignored for now, right?  So maybe add a bit of verbage in your oprofile.xml update for the TIMER event to explain that.
> 
>>> +	# Ignore configured events when running in timer mode.
>>> +	if test "$IS_TIMER" = 1; then
>>> +		NR_CHOSEN=0
>>> +	fi
>> At first, I didn't see why this was necessary here, but I see that this actually 
>> fixes the problem described in bug 
>> http://sourceforge.net/tracker/?func=detail&aid=3412988&group_id=16191&atid=116191.
> 
> Exactly. opcontrol complained about events configured in .oprofile/daemonrc after
> reloading the module with timer=1.
> 
>>> +		echo "$COUNT">  $MOUNT/0/count
>>> +		NEW_COUNT=`cat $MOUNT/0/count`
>>> +		if test "$NEW_COUNT" -lt "$COUNT"; then
>>> +			echo "Warning: Hardware sampling intervals higher than $NEW_COUNT are not supported.">&2
>>                                                                                   ^
>>
>> Shouldn't |--this be "$COUNT"?
> 
> No. After writing an invalid value into the kernel interface the value will be capped to
> the valid range and this warning is supposed to tell the user the valid maximum.
OK, I see now.
> 
>>
>>> +			echo "Value is set to $NEW_COUNT.">&2
>>> +			COUNT=$NEW_COUNT
>>> +		fi
>>> +		if test "$NEW_COUNT" -gt "$COUNT"; then
>>> +			echo "Warning: Hardware sampling intervals lower than $NEW_COUNT are not supported.">&2
>>                                                                                   ^
>>
>> Shouldn't |--this be "$COUNT"?  Actually, I don't think this should be needed if 
>> you set the min count properly in your events file (currently set to 2202).
> 
> The real range is given by the hardware and might vary. Only the kernel module will know
> about it. So I think this check is necessary here. Yes, I've put the current min value as
> reported by the machine also into the event file assuming that the real value will never
> go below this. 2202 is already too low for any real use. For now you are right this check
> will never be triggered but who knows what the hardware will report with different
> millicode levels, configurations or whatever.
OK.
> 
> Bye,
> 
> -Andreas-