Re: [PATCH net-next v3] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Mon, Nov 20, 2023 at 10:11:17AM +0100, Alexandra Winter wrote:
>On 20.11.23 04:20, Dust Li wrote:
>>> It seems to me that the purpose of conn->tx_pushing is
>>> a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
>>> b) Repeat, in case some other thread has added data to sndbuf concurrently.
>>> I agree that this patch does not change the behaviour of this function and removes an
>>> atomic_set() in the likely path.
>>> I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
>>> So how can we ever run in a concurrency situation?
>>> Is this handling of conn->tx_pushing necessary at all?
>> Hi Sandy,
>> Overall, I think you are right. But there is something we need to take care.
>> Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
>> sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
>> without checking sock_owned_by_user(), which would caused a race condition
>> because bh_lock_sock() did not honor sock_lock(). To address this issue,
>> I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
>> we now defer the transmission if sock_lock() is held by the user.
>> Therefore, there should no longer be a race condition. Nevertheless, if
>> we remove the tx_pending mechanism, we must always remember not to call
>> smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.
>> Thanks
>> Dust
>ok, I understand.
>So whoever is willing to give it a try and simplify smc_tx_sndbuf_nonempty(),
>should remember to document that requirement/precondition.
>Maybe in a Function context section of a kernel-doc function decription?
>(as described in
>Although smc_tx_sndbuf_nonempty() is not exported, this format is helpful.

I double checked this and realized that I may have missed something
previously. The original goal of introducing tx_push was to maximize the
amount of data that could be corked in order to achieve the best

__smc_tx_sndbuf_nonempty() is thread and context safe, meaning that
it can be called simultaneously in both user context and softirq without
causing any bugs, just some CPU waste. Although I think we should remove
all the atomics & locks in the data path and only use sock_lock in the

I will collaborate with Li RongQing on a new version that eliminates the

Best regards,

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux