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?
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
