linux-kernel - Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Feb 2015 05:49:07 +0008
From:	Jason Wang <jasowang@...hat.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	virtualization@...ts.linux-foundation.org, pagupta@...hat.com
Subject: Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix
 virtqueue_enable_cb() when only 1 buffers were pending



On Tue, Feb 10, 2015 at 6:18 PM, Michael S. Tsirkin <mst@...hat.com> 
wrote:
> On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote:
>>  Jason Wang <jasowang@...hat.com> writes:
>>  > We currently does:
>>  >
>>  > bufs = (avail->idx - last_used_idx) * 3 / 4;
>>  >
>>  > This is ok now since we only try to enable the delayed callbacks 
>> when
>>  > the queue is about to be full. This may not work well when there 
>> is
>>  > only one pending buffer in the virtqueue (this may be the case 
>> after
>>  > tx interrupt was enabled). Since virtqueue_enable_cb() will return
>>  > false which may cause unnecessary triggering of napis. This patch
>>  > correct this by only calculate the four thirds when bufs is not 
>> one.
>>  
>>  I mildly prefer to avoid the branch, by changing the calculation 
>> like
>>  so:
>>  
>>          /* Set bufs >= 1, even if there's only one pending buffer */
>>          bufs = (bufs + 1) * 3 / 4;
> 
> Or bus * 3/4 + 1
> 
>>  But it's not clear to me how much this happens.  I'm happy with the
>>  patch though, as currently virtqueue_enable_cb_delayed() is the same
>>  as virtqueue_enable_cb() if there's only been one buffer added.
>>  
>>  Cheers,
>>  Rusty.
> 
> But isn't this by design?
> The documentation says:
> 
>  * This re-enables callbacks but hints to the other side to delay
>  * interrupts until most of the available buffers have been processed;
> 
> Clearly, this implies that when there's one buffer it must behave
> exactly the same.
> 
> So I'm not very happy - this changes the meaning of the API without
> updating the documentation.

Think hard about this. And looks like your are right. And the patch may 
in fact cause more trouble e.g the only pending buffer were consumed 
before the final check between used idx and last_used_idx.

Will drop this.

Thanks	

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/