netdev - Re: [PATCH V2 net-next 1/7] ptr_ring: introduce batch dequeuing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a5677aa1-9475-2a1b-25f4-51c19767b9da@redhat.com>
Date:   Sat, 1 Apr 2017 13:14:10 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 net-next 1/7] ptr_ring: introduce batch dequeuing



On 2017年03月31日 22:31, Michael S. Tsirkin wrote:
> On Fri, Mar 31, 2017 at 11:52:24AM +0800, Jason Wang wrote:
>> On 2017年03月30日 21:53, Michael S. Tsirkin wrote:
>>> On Thu, Mar 30, 2017 at 03:22:24PM +0800, Jason Wang wrote:
>>>> This patch introduce a batched version of consuming, consumer can
>>>> dequeue more than one pointers from the ring at a time. We don't care
>>>> about the reorder of reading here so no need for compiler barrier.
>>>>
>>>> Signed-off-by: Jason Wang<jasowang@...hat.com>
>>>> ---
>>>>    include/linux/ptr_ring.h | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 65 insertions(+)
>>>>
>>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
>>>> index 6c70444..2be0f350 100644
>>>> --- a/include/linux/ptr_ring.h
>>>> +++ b/include/linux/ptr_ring.h
>>>> @@ -247,6 +247,22 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
>>>>    	return ptr;
>>>>    }
>>>> +static inline int __ptr_ring_consume_batched(struct ptr_ring *r,
>>>> +					     void **array, int n)
>>> Can we use a shorter name? ptr_ring_consume_batch?
>> Ok, but at least we need to keep the prefix since there's a locked version.
>>
>>
>>
>>>> +{
>>>> +	void *ptr;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < n; i++) {
>>>> +		ptr = __ptr_ring_consume(r);
>>>> +		if (!ptr)
>>>> +			break;
>>>> +		array[i] = ptr;
>>>> +	}
>>>> +
>>>> +	return i;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Note: resize (below) nests producer lock within consumer lock, so if you
>>>>     * call this in interrupt or BH context, you must disable interrupts/BH when
>>> I'd like to add a code comment here explaining why we don't
>>> care about cpu or compiler reordering. And I think the reason is
>>> in the way you use this API: in vhost it does not matter
>>> if you get less entries than present in the ring.
>>> That's ok but needs to be noted
>>> in a code comment so people use this function correctly.
>> Interesting, but I still think it's not necessary.
>>
>> If consumer is doing a busy polling, it will eventually get the entries. If
>> the consumer need notification from producer, it should drain the queue
>> which means it need enable notification before last try of consuming call,
>> otherwise it was a bug. The batch consuming function in this patch can
>> guarantee return at least one pointer if there's many, this looks sufficient
>> for the correctness?
>>
>> Thanks
> You ask for N entries but get N-1. This seems to imply the
> ring is now empty. Do we guarantee this?

I think consumer can not assume ring is empty consider producer can 
produce at the same time. It need enable notification and do another 
poll in this case.

Thanks