Messages in this thread | | | Date | Thu, 24 Jan 2013 12:37:25 +0200 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH V6 3/3] tuntap: allow polling/writing/reading when detached |
| |
On Thu, Jan 24, 2013 at 06:12:44PM +0800, Jason Wang wrote: > On 01/17/2013 09:16 AM, Jason Wang wrote: > > On 01/17/2013 01:03 AM, Michael S. Tsirkin wrote: > >> On Wed, Jan 16, 2013 at 11:44:38PM +0800, Jason Wang wrote: > >>> We forbid polling, writing and reading when the file were detached, this may > >>> complex the user in several cases: > >>> > >>> - when guest pass some buffers to vhost/qemu and then disable some queues, > >>> host/qemu needs to do its own cleanup on those buffers which is complex > >>> sometimes. We can do this simply by allowing a user can still write to an > >>> disabled queue. Write to an disabled queue will cause the packet pass to the > >>> kernel and read will get nothing. > >>> - align the polling behavior with macvtap which never fails when the queue is > >>> created. This can simplify the polling errors handling of its user (e.g vhost) > >>> > >>> In order to achieve this, tfile->tun were not assign to NULL when detached. And > >>> tfile->tun were converted to be RCU protected in order to let the data path can > >>> check whether the file is deated in a lockless manner. This will be used to > >>> prevent the flow caches from being updated for a detached queue. > >>> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>> --- > >>> drivers/net/tun.c | 45 ++++++++++++++++++++++++++------------------- > >>> 1 files changed, 26 insertions(+), 19 deletions(-) > >>> > >>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c > >>> index c81680d..ec539a9 100644 > >>> --- a/drivers/net/tun.c > >>> +++ b/drivers/net/tun.c > >>> @@ -139,7 +139,7 @@ struct tun_file { > >>> unsigned int flags; > >>> u16 queue_index; > >>> struct list_head next; > >>> - struct tun_struct *detached; > >>> + struct tun_struct __rcu *detached; > >>> }; > >>> > >>> struct tun_flow_entry { > >>> @@ -295,11 +295,12 @@ static void tun_flow_cleanup(unsigned long data) > >>> } > >>> > >>> static void tun_flow_update(struct tun_struct *tun, u32 rxhash, > >>> - u16 queue_index) > >>> + struct tun_file *tfile) > >>> { > >>> struct hlist_head *head; > >>> struct tun_flow_entry *e; > >>> unsigned long delay = tun->ageing_time; > >>> + u16 queue_index = tfile->queue_index; > >>> > >>> if (!rxhash) > >>> return; > >>> @@ -308,7 +309,7 @@ static void tun_flow_update(struct tun_struct *tun, u32 rxhash, > >>> > >>> rcu_read_lock(); > >>> > >>> - if (tun->numqueues == 1) > >>> + if (tun->numqueues == 1 || rcu_dereference(tfile->detached)) > >>> goto unlock; > >>> > >>> e = tun_flow_find(head, rxhash); > >> Sorry, still an issue with this one. > > No problem, thanks for the checking. > >> u16 index = tfile->queue_index; > >> BUG_ON(index >= tun->numqueues); > >> dev = tun->dev; > >> > >> rcu_assign_pointer(tun->tfiles[index], > >> tun->tfiles[tun->numqueues - 1]); > >> rcu_assign_pointer(tfile->tun, NULL); > >> ntfile = rtnl_dereference(tun->tfiles[index]); > >> ntfile->queue_index = index; > >> > >> --tun->numqueues; > >> if (clean) > >> sock_put(&tfile->sk); > >> else > >> tun_disable_queue(tun, tfile); > >> > >> You should first disable queue then synchronize network > >> only then play with tfiles array. > >> As it is you might have removed file from array but > >> did not set detached flag yet, so queue_index > >> above is stable. > > I think the code is ok here. With this patch, before synchronize_net(), > > the only thing we do for the tfile that will be detached is to set the > > tfile->detached (tun_disable_queue), and the queue_index is kept > > unchanged. So if the data path don't see the new value of detached, it > > still can treat the tfile is undetached and do the sending and receiving > > as usual. We only do the cleanup after the synchronization which all > > reader are guaranteed to see the new detached value. > > > > For the tfile that will be moved to the new place, some (should be very > > little) OOO will occur which I think is acceptable and can be optimized > > in the future. > > Having a thought about this patch, looks like it's suboptimal since: > > - If we can make sure no packets were sent to the disabled queue and > stop the vhost thread during switching (then it can flush). There's no > need for this patch.
This assumes synchronization in userspace/vhost, this will make datapath slower without real need.
> - Allowing writing/polling to a detached fd
It's not a detached fd - it's attached to tun. We just disabled receiving packets on it.
> is strange and can hide the > bugs of userspace / guest driver.
That's a good thing, you don't want a fragile interface.
> > So looks like we'd better drop this patch?
I actually think it's the right approach. And since you clarified I think the patch is allright.
> >> On enable, clear detached last thing. > >> > >>> @@ -384,16 +385,16 @@ static void tun_set_real_num_queues(struct tun_struct *tun) > >>> > >>> static void tun_disable_queue(struct tun_struct *tun, struct tun_file *tfile) > >>> { > >>> - tfile->detached = tun; > >>> + rcu_assign_pointer(tfile->detached, tun); > >>> list_add_tail(&tfile->next, &tun->disabled); > >>> ++tun->numdisabled; > >>> } > >>> > >>> static struct tun_struct *tun_enable_queue(struct tun_file *tfile) > >>> { > >>> - struct tun_struct *tun = tfile->detached; > >>> + struct tun_struct *tun = rtnl_dereference(tfile->detached); > >>> > >>> - tfile->detached = NULL; > >>> + rcu_assign_pointer(tfile->detached, NULL); > >>> list_del_init(&tfile->next); > >>> --tun->numdisabled; > >>> return tun; > >>> @@ -402,26 +403,27 @@ static struct tun_struct *tun_enable_queue(struct tun_file *tfile) > >>> static void __tun_detach(struct tun_file *tfile, bool clean) > >>> { > >>> struct tun_file *ntfile; > >>> - struct tun_struct *tun; > >>> + struct tun_struct *tun, *detached; > >>> struct net_device *dev; > >>> > >>> tun = rtnl_dereference(tfile->tun); > >>> + detached = rtnl_dereference(tfile->detached); > >>> > >>> - if (tun) { > >>> + if (tun && !detached) { > >>> u16 index = tfile->queue_index; > >>> BUG_ON(index >= tun->numqueues); > >>> dev = tun->dev; > >>> > >>> rcu_assign_pointer(tun->tfiles[index], > >>> tun->tfiles[tun->numqueues - 1]); > >>> - rcu_assign_pointer(tfile->tun, NULL); > >>> ntfile = rtnl_dereference(tun->tfiles[index]); > >>> ntfile->queue_index = index; > >>> > >>> --tun->numqueues; > >>> - if (clean) > >>> + if (clean) { > >>> + rcu_assign_pointer(tfile->tun, NULL); > >>> sock_put(&tfile->sk); > >>> - else > >>> + } else > >>> tun_disable_queue(tun, tfile); > >>> > >>> synchronize_net(); > >>> @@ -429,7 +431,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean) > >>> /* Drop read queue */ > >>> skb_queue_purge(&tfile->sk.sk_receive_queue); > >>> tun_set_real_num_queues(tun); > >>> - } else if (tfile->detached && clean) { > >>> + } else if (detached && clean) { > >>> tun = tun_enable_queue(tfile); > >>> sock_put(&tfile->sk); > >>> } > >>> @@ -466,6 +468,10 @@ static void tun_detach_all(struct net_device *dev) > >>> rcu_assign_pointer(tfile->tun, NULL); > >>> --tun->numqueues; > >>> } > >>> + list_for_each_entry(tfile, &tun->disabled, next) { > >>> + wake_up_all(&tfile->wq.wait); > >>> + rcu_assign_pointer(tfile->tun, NULL); > >>> + } > >>> BUG_ON(tun->numqueues != 0); > >>> > >>> synchronize_net(); > >>> @@ -496,7 +502,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file) > >>> goto out; > >>> > >>> err = -EINVAL; > >>> - if (rtnl_dereference(tfile->tun)) > >>> + if (rtnl_dereference(tfile->tun) && !rtnl_dereference(tfile->detached)) > >>> goto out; > >>> > >>> err = -EBUSY; > >>> @@ -504,7 +510,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file) > >>> goto out; > >>> > >>> err = -E2BIG; > >>> - if (!tfile->detached && > >>> + if (!rtnl_dereference(tfile->detached) && > >>> tun->numqueues + tun->numdisabled == MAX_TAP_QUEUES) > >>> goto out; > >>> > >>> @@ -521,7 +527,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file) > >>> rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile); > >>> tun->numqueues++; > >>> > >>> - if (tfile->detached) > >>> + if (rtnl_dereference(tfile->detached)) > >>> tun_enable_queue(tfile); > >>> else > >>> sock_hold(&tfile->sk); > >>> @@ -1195,7 +1201,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > >>> tun->dev->stats.rx_packets++; > >>> tun->dev->stats.rx_bytes += len; > >>> > >>> - tun_flow_update(tun, rxhash, tfile->queue_index); > >>> + tun_flow_update(tun, rxhash, tfile); > >>> return total_len; > >>> } > >>> > >>> @@ -1552,7 +1558,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) > >>> struct net_device *dev; > >>> int err; > >>> > >>> - if (tfile->detached) > >>> + if (rtnl_dereference(tfile->detached)) > >>> return -EINVAL; > >>> > >>> dev = __dev_get_by_name(net, ifr->ifr_name); > >>> @@ -1796,7 +1802,7 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > >>> rtnl_lock(); > >>> > >>> if (ifr->ifr_flags & IFF_ATTACH_QUEUE) { > >>> - tun = tfile->detached; > >>> + tun = rtnl_dereference(tfile->detached); > >>> if (!tun) { > >>> ret = -EINVAL; > >>> goto unlock; > >>> @@ -1807,7 +1813,8 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr) > >>> ret = tun_attach(tun, file); > >>> } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) { > >>> tun = rtnl_dereference(tfile->tun); > >>> - if (!tun || !(tun->flags & TUN_TAP_MQ)) > >>> + if (!tun || !(tun->flags & TUN_TAP_MQ) || > >>> + rtnl_dereference(tfile->detached)) > >>> ret = -EINVAL; > >>> else > >>> __tun_detach(tfile, false); > >>> -- > >>> 1.7.1 > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> Please read the FAQ at http://www.tux.org/lkml/ > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/
| |