On Tue, Oct 20, 2009 at 04:14:21PM -0700, Joe Eykholt wrote:
> Vasu Dev wrote:
>> On Tue, 2009-10-20 at 20:54 +0200, Christof Schmitt wrote:
>>> On Fri, Oct 16, 2009 at 04:08:24PM -0700, Vasu Dev wrote:
>>>> -v3
>>>> Moves max_queue_depth initialization after slave_configure is
>>>> called from after slave_alloc calling done. Also adjusted
>>>> max_queue_depth check to skip ramp up if current queue_depth
>>>> is >= max_queue_depth.
>>>>
>>>> Signed-off-by: Vasu Dev <vasu.dev@xxxxxxxxx>
>>> Looks good to me. I ran some tests on s390 and the queue depth counter
>>> increased until hitting the defined maximum.
>>>
>>
>> Good.
>>
>>>> @@ -779,6 +781,36 @@ static struct device_attribute sdev_attr_queue_depth_rw =
>>>> sdev_store_queue_depth_rw);
>>>>
>>>> static ssize_t
>>>> +sdev_show_queue_ramp_up_period(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct scsi_device *sdev;
>>>> + sdev = to_scsi_device(dev);
>>>> + return snprintf(buf, 20, "%lu\n", sdev->queue_ramp_up_period);
>>>> +}
>>>> +
>>>> +static ssize_t
>>>> +sdev_store_queue_ramp_up_period(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + struct scsi_device *sdev = to_scsi_device(dev);
>>>> + unsigned long period;
>>>> +
>>>> + if (strict_strtoul(buf, 10, &period))
>>>> + return -EINVAL;
>>>> +
>>>> + sdev->queue_ramp_up_period = period;
>>>> + return period;
>>>> +}
>>> [...]
>>>> + unsigned long queue_ramp_up_period; /* ramp up period in jiffies */
>>>> +#define SCSI_DEFAULT_RAMP_UP_PERIOD (120 * HZ)
>>> Only a small inconvenience i guess: The sysfs attribute shows the
>>> ramp-up-period in jiffies. On my system with HZ==100 the default is
>>> 12000 and i was wondering if this would be milliseconds or something
>>> related. If HZ changes, the unit of the ramp-up-period also changes.
>>>
>>> I would prefer having seconds or milliseconds in sysfs and only using
>>> jiffies internally.
>>>
>>
>> Added timestamp comparison checks are straightforward without any
>> additional conversion on each check since added timestamps and
>> queue_ramp_up_period both are jiffies, so works better this way with
>> stored queue_ramp_up_period in jiffies.
>>
>> I do see your point as well but jiffies unit is also common in Linux.
>> However if you or some other feels strongly to change this to ms or
>> seconds unit then I could change store or show sysfs functions to handle
>> this value in ms or second unit while storing it in jiffies in
>> queue_ramp_up_period.
>>
>> Vasu
>
> I agree with Christof on this, HZ may be 100 on some machines and 1000
> on others. Management tools (if they ever adjust this) would like
> standard units, so it should be in ms or seconds, and I prefer ms.
> I would just change the sysfs functions as you say, and use jiffies internally.
I would also vote for ms in sysfs and using jiffies internally. This
should be fairly easy using the functions jiffies_to_msecs and
msecs_to_jiffies.
Christof
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
