在 2024/11/5 22:44, Dust Li 写道:
> On 2024-11-05 11:19:38, liqiang wrote:
>> [...]
>> I tested the time-consuming comparison of this function
>> under multiple connections based on redis-benchmark
>> (test in smc loopback-ism mode):
>> The function 'smc_buf_get_slot' takes less time when a
>> new SMC link is established:
>> 1. 5us->100ns (when there are 200 active links);
>> 2. 30us->100ns (when there are 1000 active links).
>> [...]
>> -/* try to reuse a sndbuf or rmb description slot for a certain
>> - * buffer size; if not available, return NULL
>> - */
>> -static struct smc_buf_desc *smc_buf_get_slot(int compressed_bufsize,
>> - struct rw_semaphore *lock,
>> - struct list_head *buf_list)
>> +/* use lock less list to save and find reuse buf desc */
>> +static struct smc_buf_desc *smc_buf_get_slot_free(struct llist_head *buf_llist)
>> {
>> - struct smc_buf_desc *buf_slot;
>> + struct smc_buf_desc *buf_free;
>> + struct llist_node *llnode;
>>
>> - down_read(lock);
>> - list_for_each_entry(buf_slot, buf_list, list) {
>> - if (cmpxchg(&buf_slot->used, 0, 1) == 0) {
>> - up_read(lock);
>> - return buf_slot;
>> - }
>> - }
>> - up_read(lock);
>> - return NULL;
>> + /* lock-less link list don't need an lock */
>> + llnode = llist_del_first(buf_llist);
>> + if (!llnode)
>> + return NULL;
>> + buf_free = llist_entry(llnode, struct smc_buf_desc, llist);
>> + WRITE_ONCE(buf_free->used, 1);
>> + return buf_free;
>
> Sorry for the late reply.
>
> It looks this is not right here.
>
> The rw_semaphore here is not used to protect against adding/deleting
> the buf_list since we don't even add/remove elements on the buf_list.
> The cmpxchg already makes sure only one will get an unused smc_buf_desc.
I first came up with the idea of optimizing because this function needs to
traverse all rmbs/sndbufs, which includes all active links and is a waste
of time and unnecessary.
Changing to an llist linked list implementation can ensure that a free buf_slot
with a 'used' mark of 0 can be directly obtained every time, without the need
to start traversing from the first element of the rmbs/sndbufs linked list.
>
> Removing the down_read()/up_read() would cause mapping/unmapping link
> on the link group race agains the buf_slot alloc/free here. For exmaple
> _smcr_buf_map_lgr() take the write lock of the rw_semaphore.
Read from the relevant code, here only a buf_slot is found in the
down_read/up_read and 'used' is set to 0, while the 'used' is read
in other down_write/up_write code.
so I have two questions:
1. Is the read lock of rw_semaphore necessary here? (The read lock here
is mutually exclusive with the write lock elsewhere, what is guaranteed
should be that in the critical section of the write lock, all
'smc_buf_desc->used' statuses in rmbs/sndbufs will not change.)
2. If is necessary, can we add it in new implement of this patch, like this?
```
{
struct smc_buf_desc *buf_free;
struct llist_node *llnode;
/* lock-less link list don't need an lock */
llnode = llist_del_first(buf_llist);
if (!llnode)
return NULL;
buf_free = llist_entry(llnode, struct smc_buf_desc, llist);
up_read(lock);
WRITE_ONCE(buf_free->used, 1);
down_read(lock);
return buf_free;
}
```
I think this can also ensure that all 'used' marks remain unchanged during
the write lock process.
Anyway, use llist to manage free rmbs/sndbufs is a better choice than traverse,
and it doesn't conflict with useing or not using rw_semaphore. :)
>
> But I agree the lgr->rmbs_lock/sndbufs_lock should be improved. Would
> you like digging into it and improve the usage of the lock here ?
>
Maybe I can try it, I need to spend some time to read in detail the
code used in every place of this lock.
Thanks for taking the time to read this email. ;-)
--
Best regards,
Li Qiang
