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 :)
>
> Thanks
>
>
>>
>> Jason
>>
>> --
>> 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
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
