On Thu, Mar 28, 2019 at 05:23:33PM +0200, Leon Romanovsky wrote: > On Wed, Mar 27, 2019 at 01:27:57PM +0000, Bernard Metzler wrote: > > > > >To: "Bernard Metzler" <bmt@xxxxxxxxxxxxxx> > > >From: "Leon Romanovsky" <leon@xxxxxxxxxx> > > >Date: 03/26/2019 11:16AM > > >Cc: linux-rdma@xxxxxxxxxxxxxxx > > >Subject: Re: [PATCH v6 03/13] SIW network and RDMA core interface > > > > > >On Mon, Mar 25, 2019 at 06:10:37PM +0100, Bernard Metzler wrote: > > >> Signed-off-by: Bernard Metzler <bmt@xxxxxxxxxxxxxx> > > <...> > > > >> + kthread_bind(siw_tx_thread[cpu], cpu); > > >> + > > >> + wake_up_process(siw_tx_thread[cpu]); > > >> + assigned++; > > >> + } > > >> + return assigned; > > >> +} > > > > > >I'm not so sure that this kthread_create over all CPUs is legitimate > > >usage of kernel threads. Especially given the fact that netdev > > >doesn't > > >have it except liquidio driver, which was in very bad shape last time > > >when I looked on it. So having it in liquidio is big red flag for me. > > > > > > > I understand your concern. Earlier siw was using work queues, but > > it turned out to be a rather bad fit for low delay transmits. With some > > profiling we found that under load the system spends almost one third of > > its time at those work queue spinlocks. We decided to implement a > > very simple lock-less queue, which helped a lot with delay and overall > > load. We looked at tasklet's as well. This is what rxe is > > doing. We do not think it is the right use of tasklet's. We better > > want to transmit out of an interruptible process context, and > > not atomic. > > This code will create the unreasonable large number of kernel threads on > my machine with large count of CPUs. > > <...> > > > >> + base_dev->owner = THIS_MODULE; > > > > > >Why is it needed? > > > > To prevent siw from being unloaded while file operations on the device > > are in progress. Aren't all drivers setting it? > > Yes, but you are not doing any direct file operations, everything is > supposed to be passed through IB/core (maybe wrong here). Drivers are supposed to set it. The core uses it to manage the module refcount lock on the driver. The potentially confusing bit is that we don't rely on the refcount lock for any correctness, it is just a user experience thing so that rmmod doesn't hang forever. However it should be migrated into the ib_device_ops static structure the driver has instead of open coded like this. Jason
