Re: [PATCH net-next v3 5/5] virtio-net: initial rx sysfs support, export mergeable rx buffer size
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2014-01-16 18:50:51
Also in:
netdev
On Thu, Jan 16, 2014 at 10:04:41AM -0800, Eric Dumazet wrote:
On Thu, 2014-01-16 at 09:27 -0800, Michael Dalton wrote:quoted
Sorry, just realized - I think disabling NAPI is necessary but not sufficient. There is also the issue that refill_work() could be scheduled. If refill_work() executes, it will re-enable NAPI. We'd need to cancel the vi->refill delayed work to prevent this AFAICT, and also ensure that no other function re-schedules vi->refill or re-enables NAPI (virtnet_open/close, virtnet_set_queues, and virtnet_freeze/restore). How is the following sequence of operations: rtnl_lock(); cancel_delayed_work_sync(&vi->refill); napi_disable(&rq->napi); read rq->mrg_avg_pkt_len virtnet_enable_napi(); rtnl_unlock(); Additionally, if we disable NAPI when reading this file, perhaps the permissions should be changed to 400 so that an unprivileged user cannot temporarily disable network RX processing by reading these sysfs files. Does that sound reasonable?I think all this complexity makes no sense to me. Who cares of sysfs reading a value that might be updated ? This is purely a debugging utility. As soon as you read the value, it might already have changed anyway. Its a integer, just read it without special care.
That's fine too as far as I'm concerned.
quoted hunk ↗ jump to hunk
atomic_read() has also same 'problem', and we do not care. Make sure that a recompute (aka ewma_add()) does not store intermediate wrong values, by using ACCESS_ONCE(), and thats enough. No need for the seqcount overhead.diff --git a/lib/average.c b/lib/average.c index 99a67e662b3c..044e0b7f28a8 100644 --- a/lib/average.c +++ b/lib/average.c@@ -53,8 +53,10 @@ EXPORT_SYMBOL(ewma_init); */ struct ewma *ewma_add(struct ewma *avg, unsigned long val) { - avg->internal = avg->internal ? - (((avg->internal << avg->weight) - avg->internal) + + unsigned long internal = ACCESS_ONCE(avg->internal); + + ACCESS_ONCE(avg->internal) = internal ? + (((internal << avg->weight) - internal) + (val << avg->factor)) >> avg->weight : (val << avg->factor); return avg;