On 18/03/2018 16:00, Leon Romanovsky wrote: > On Sun, Mar 18, 2018 at 01:33:31PM +0000, Mark Bloch wrote: >> Today the get_hw_stats() API looks like this: >> >> get_hw_stats(struct ib_device *ibdev, struct rdma_hw_stats *stats, >> u8 port, int index) >> >> The driver gets a rdma_hw_stats structure, a port and an index and is given the >> possibility to: >> >> 1) Fill only the counter at location index >> 2) Fill all the counters >> >> Filling the counters is done like so: >> stats->value[i], 0 <= i < stats->num_counters >> The stats structure is shared between all the counters. Today all the drivers >> that implement get_hw_stats() always fill all the counters regardless of the >> index given. >> >> This may lead to the following scenario if we have multiple concurrent reads >> of counters, cpu 0 (tries to read counter at index 0) cpu 1 (tries to read >> counter at index 1) >> >> CPU 0 CPU 1 >> driver: stats->value[0] = x; | .... >> .... | .... >> .... | .... >> ib_core: return stats->value[0] to user | driver: stats->value[0] = y; >> >> We end up with read/write to the same location at the same time. >> Which can lead to providing an incorrect value to the user. This RFC introduces >> a lock to protect against that. >> > > As I said in offline discussion, it solves non-existent problem. > SW (kernel) doesn't guarantee that counters are correct every time you > snapshot them. All CPUs are calling to FW prior to counter update, so > they will write correct data, just a little bit out-of-sync. You assume here the driver/compiler assigns only once to stats->value[i] (which you can't) and that read/write are atomic (which you also can't). You can totally get garbage values today (and not just out-of-sync) > > And in 64bits systems, such writes to tats->value will be atomic. > > Thanks > > >> Mark Bloch (1): >> IB/core: Protect against concurrent access to hardware stats >> >> drivers/infiniband/core/sysfs.c | 10 ++++++++-- >> include/rdma/ib_verbs.h | 1 + >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> -- >> 1.8.4.3 >> >> -- >> 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
