On 19/03/2018 18:09, Jason Gunthorpe wrote:
> On Mon, Mar 19, 2018 at 06:06:46PM +0200, Mark Bloch wrote:
>>
>>
>> On 19/03/2018 17:59, Leon Romanovsky wrote:
>>> On Mon, Mar 19, 2018 at 09:13:02AM -0600, Jason Gunthorpe wrote:
>>>> On Mon, Mar 19, 2018 at 12:08:09PM +0200, Leon Romanovsky wrote:
>>>>>> 794 static ssize_t show_hw_stats(struct kobject *kobj, struct attribute *attr,
>>>>>> 795 char *buf)
>>>>>> 796 {
>>>>>> 797 struct ib_device *dev;
>>>>>> 798 struct ib_port *port;
>>>>>> 799 struct hw_stats_attribute *hsa;
>>>>>> 800 struct rdma_hw_stats *stats;
>>>>>> 801 int ret;
>>>>>> 802
>>>>>> 803 hsa = container_of(attr, struct hw_stats_attribute, attr);
>>>>>> 804 if (!hsa->port_num) {
>>>>>> 805 dev = container_of((struct device *)kobj,
>>>>>> 806 struct ib_device, dev);
>>>>>> 807 stats = dev->hw_stats;
>>>>>> 808 } else {
>>>>>> 809 port = container_of(kobj, struct ib_port, kobj);
>>>>>> 810 dev = port->ibdev;
>>>>>> 811 stats = port->hw_stats;
>>>>>
>>>>> Mark pointed to me that "stats" is actually pointer to shared memory.
>>>>
>>>> Yes it is.
>>>>
>>>> So either stats should be made thread local here, or we need the lock.
>>>>
>>>> You can't rely on ordinary writes to shared memory to do anything
>>>> sane, only atomic writes have sensible properties.
>>>>
>>>> I can't think of any reason why adding this lock would be bad?
>>>
>>> It is not bad, but I wanted to push Mark to think about thread safe
>>> solution. I don't like the fact that all callers of get_hw_state will
>>> need to put lock around it.
>>
>> Like I said off list, this is done in order:
>>
>> 1) Avoid allocations on each query.
>> 2) Provide caching infrastructure for all drivers.
>>
>> If we don't care about those, I'm sure I can come up with a different solution.
>> Is it really worth the trouble :)
>
> You said no drivers used the caching infrastructure though?
>
Where?
What I said that all the drivers always fill all the counters.
The caching is done for them by ib_core, see:
show_hw_stats()
Which called:
update_hw_stats()
And there we do:
if (time_is_after_eq_jiffies(stats->timestamp + stats->lifespan))
return 0;
which means: "Don't query the driver, use the cached value"
> Jason
>
Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
