On Fri, Jun 11, 2021 at 07:25:46AM +0000, Haakon Bugge wrote:

quoted

On 7 Jun 2021, at 14:50, Jason Gunthorpe [off-list ref] wrote:

On Mon, Jun 07, 2021 at 02:39:45PM +0200, Greg KH wrote:

quoted

On Mon, Jun 07, 2021 at 09:14:11AM -0300, Jason Gunthorpe wrote:

quoted

On Mon, Jun 07, 2021 at 12:25:03PM +0200, Greg KH wrote:

quoted

On Mon, Jun 07, 2021 at 11:17:35AM +0300, Leon Romanovsky wrote:

quoted

From: Jason Gunthorpe <jgg@nvidia.com>

This code is trying to attach a list of counters grouped into 4 groups to
the ib_port sysfs. Instead of creating a bunch of kobjects simply express
everything naturally as an ib_port_attribute and add a single
attribute_groups list.

Remove all the naked kobject manipulations.

Much nicer.

But why do you need your counters to be atomic in the first place?  What
are they counting that requires this?  

The write side of the counter is being updated from concurrent kernel
threads without locking, so this is an atomic because the write side
needs atomic_add().

So the atomic write forces a lock :(

Of course, but a single atomic is cheaper than the double atomic in a
full spinlock.

quoted

quoted

Making them a naked u64 will cause significant corruption on the write
side, and packet counters that are not accurate after quiescence are
not very useful things.

How "accurate" do these have to be?

They have to be accurate. They are networking packet counters. What is
the point of burning CPU cycles keeping track of inaccurate data?

Consider a CPU with a 32-bit wide datapath to memory, which reads and writes the most significant 4-byte word first:

What CPU is that?

    Memory                   CPU1                   CPU2
MSW         LSW        MSW         LSW        MSW         LSW
0x0  0xffffffff
0x0  0xffffffff        0x0
0x0  0xffffffff        0x0  0xffffffff
0x0  0xffffffff        0x1         0x0                         cpu1 has incremented its register
0x1  0xffffffff        0x1         0x0                         cpu1 has written msw
0x1  0xffffffff        0x1         0x0        0x1              cpu2 has read msw
0x1  0xffffffff        0x1         0x0        0x1  0xffffffff
0x1         0x0        0x1         0x0        0x2         0x0
0x2         0x0        0x1         0x0        0x2         0x0
0x2         0x0        0x1         0x0        0x2         0x0


I would say that 0x200000000 vs. 0x100000001 is more than inaccurate!

True, then maybe these should just be 32bit counters :)

thanks,

greg k-h