-----"Leon Romanovsky" <leon@xxxxxxxxxx> wrote: -----
>To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>From: "Leon Romanovsky" <leon@xxxxxxxxxx>
>Date: 03/28/2019 04:23PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v6 03/13] SIW network and RDMA core interface
>
>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.
>
It results in one tx thread per physical (not HT) core.
But these are sleeping dogs, if nothing is to be done.
><...>
>
>> >> + 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?
>
OK let me re-order.
>Thanks
>
[attachment "signature.asc" removed by Bernard Metzler/Zurich/IBM]
