On Mon, Mar 19, 2018 at 06:15:10PM +0200, Mark Bloch wrote:
>
>
> 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"
Okay, I mis understood, now I recall this section..
Yes, we really do need this lock, surprised it was missed, so obvious :(
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
