On Wed, Mar 27, 2019 at 01:27:57PM +0000, Bernard Metzler wrote:
> -----"Leon Romanovsky" <leon@xxxxxxxxxx> 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).
<...>
> >> + int rv;
> >> +
> >> + base_dev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_SIW);
> >> + if (base_dev) {
> >> + ib_device_put(base_dev);
> >> + rv = -EEXIST;
> >> + goto error;
> >
> >Return directly.
> >
> >> + }
> >> + if (!siw_dev_qualified(netdev)) {
> >> + rv = -EINVAL;
> >> + goto error;
> >
> >You need to do ib_device_put(base_dev); here.
> >
> Hmm, no, it's NULL and we are going to set it up next
> in siw_device_create() since it doesn't exist yet.
Ohh, I see, it is a little bit unusual pattern to ball out
if base_dev is not null. What can prevent from creating
base_dev immediately after your checks here?
Thanks
Attachment:
signature.asc
Description: PGP signature
