* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-09 7:20 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911091647.29655.rusty@rustcorp.com.au>
On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote:
> On Sun, 8 Nov 2009 10:05:16 pm Michael S. Tsirkin wrote:
> > On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote:
> > > > +/* Caller must have TX VQ lock */
> > > > +static void tx_poll_stop(struct vhost_net *net)
> > > > +{
> > > > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> > > > + return;
> > >
> > > likely? Really?
> >
> > Hmm ... yes. tx poll stop is called on each packet (as long as we do not
> > fill up 1/2 backend queue), the first call will stop polling
> > the rest checks state and does nothing.
> >
> > This is because we normally do not care when the message has left the
> > queue in backend device: we tell backend to send it and forget. We only
> > start polling when backend tx queue fills up.
>
> OK, good.
>
> > > > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
> > > > +{
> > > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > > > + sizeof(struct virtio_net_hdr) : 0;
> > > > + int i;
> > > > + mutex_lock(&n->dev.mutex);
> > > > + n->dev.acked_features = features;
> > >
> > > Why is this called "acked_features"? Not just "features"? I expected
> > > to see code which exposed these back to userspace, and didn't.
> >
> > Not sure how do you mean. Userspace sets them, why
> > does it want to get them exposed back?
>
> There's something about the 'acked' which rubs me the wrong way.
> "enabled_features" is perhaps a better term than "acked_features"; "acked"
> seems more a user point-of-view, "enabled" seems more driver POV?
>
> set_features matches your ioctl names, but it sounds like a fn name :(
Hmm. Are you happy with the ioctl name? If yes I think being consistent
with that is important.
> It's marginal. And 'features' is shorter than both.
I started with this but I was always getting confused whether this
includes all features or just acked features. I'll go with
enabled_features.
> > > > + switch (ioctl) {
> > > > + case VHOST_SET_VRING_NUM:
> > >
> > > I haven't looked at your userspace implementation, but does a generic
> > > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> > > sense? It'd be simpler here,
> >
> > Not by much though, right?
> >
> > > but not sure if it'd be simpler to use?
> >
> > The problem is with VHOST_SET_VRING_BASE as well. I want it to be
> > separate because I want to make it possible to relocate e.g. used ring
> > to another address while ring is running. This would be a good debugging
> > tool (you look at kernel's used ring, check descriptor, then update
> > guest's used ring) and also possibly an extra way to do migration. And
> > it's nicer to have vring size separate as well, because it is
> > initialized by host and never changed, right?
>
> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume.
Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a
cached value for notify on empty, so what this does is clear the value.
What exactly do you refer to when you say "this looks wrong"?
This could trigger an extra notification if I ever called
trigger_irq without get first. As I don't, it in fact has no effect.
> The only reason it's not in the ring itself
> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.
Yes, I remember that patch. There seems to be little point though, at
this stage.
>
> I would drop avail_idx altogether: get_user is basically free, and
> simplifies a lot. As most state is in the ring, all you need is an
> ioctl to save/restore the last_avail_idx.
avail_idx is there for notify on empty: I had this thought that it's
better to leave the avail cache line alone when we are triggering
interrupt to avoid bouncing it around if guest is updating it meanwhile
on another CPU, and I think my testing showed that it helped
performance, but could be a mistake. You don't believe this can help?
> > We could merge DESC, AVAIL, USED, and it will reduce the amount of code
> > in userspace. With both base, size and fds separate, it seemed a bit
> > more symmetrical to have desc/avail/used separate as well.
> > What's your opinion?
>
> Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.
OK, I'll do this.
> > > For future reference, this is *exactly* the kind of thing which would have
> > > been nice as a followup patch. Easy to separate, easy to review, not critical
> > > to the core.
> >
> > Yes. It's not too late to split it out though: should I do it yet?
>
> Only if you're feeling enthused. It's lightly reviewed now.
Not really :) I'll keep this in mind for the future.
Thanks!
> Cheers,
> Rusty.
>
> virtio: put last_used and last_avail index into ring itself.
>
> Generally, the other end of the virtio ring doesn't need to see where
> you're up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, if you want to save and restore a virtio_ring, but you're
> not the consumer because the kernel is using it directly.
>
> Fortunately, we have room to expand: the ring is always a whole number
> of pages and there's hundreds of bytes of padding after the avail ring
> and the used ring, whatever the number of descriptors (which must be a
> power of 2).
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> drivers/virtio/virtio_ring.c | 23 +++++++++++++++--------
> include/linux/virtio_ring.h | 12 +++++++++++-
> 2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -71,9 +71,6 @@ struct vring_virtqueue
> /* Number we've added since last sync. */
> unsigned int num_added;
>
> - /* Last used index we've seen. */
> - u16 last_used_idx;
> -
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> @@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
>
> static inline bool more_used(const struct vring_virtqueue *vq)
> {
> - return vq->last_used_idx != vq->vring.used->idx;
> + return vring_last_used(&vq->vring) != vq->vring.used->idx;
> }
>
> static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_used_elem *u;
> void *ret;
> unsigned int i;
>
> @@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
> return NULL;
> }
>
> - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
> - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> + u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
> + i = u->id;
> + *len = u->len;
> + /* Make sure we don't reload i after doing checks. */
> + rmb();
>
> if (unlikely(i >= vq->vring.num)) {
> BAD_RING(vq, "id %u out of range\n", i);
> @@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
> /* detach_buf clears data, so grab it now. */
> ret = vq->data[i];
> detach_buf(vq, i);
> - vq->last_used_idx++;
> + vring_last_used(&vq->vring)++;
> +
> END_USE(vq);
> return ret;
> }
> @@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
> vq->vq.name = name;
> vq->notify = notify;
> vq->broken = false;
> - vq->last_used_idx = 0;
> vq->num_added = 0;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> @@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
>
> vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
>
> + /* We publish indices whether they offer it or not: if not, it's junk
> + * space anyway. But calling this acknowledges the feature. */
> + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
> +
> /* No callback? Tell other side not to bother us. */
> if (!callback)
> vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> @@ -443,6 +448,8 @@ void vring_transport_features(struct vir
> switch (i) {
> case VIRTIO_RING_F_INDIRECT_DESC:
> break;
> + case VIRTIO_RING_F_PUBLISH_INDICES:
> + break;
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -29,6 +29,9 @@
> /* We support indirect buffer descriptors */
> #define VIRTIO_RING_F_INDIRECT_DESC 28
>
> +/* We publish our last-seen used index at the end of the avail ring. */
> +#define VIRTIO_RING_F_PUBLISH_INDICES 29
> +
> /* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
> struct vring_desc
> {
> @@ -87,6 +90,7 @@ struct vring {
> * __u16 avail_flags;
> * __u16 avail_idx;
> * __u16 available[num];
> + * __u16 last_used_idx;
> *
> * // Padding to the next align boundary.
> * char pad[];
> @@ -95,6 +99,7 @@ struct vring {
> * __u16 used_flags;
> * __u16 used_idx;
> * struct vring_used_elem used[num];
> + * __u16 last_avail_idx;
> * };
> */
> static inline void vring_init(struct vring *vr, unsigned int num, void *p,
> @@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
> {
> return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
> + align - 1) & ~(align - 1))
> - + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
> + + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
> }
>
> +/* We publish the last-seen used index at the end of the available ring, and
> + * vice-versa. These are at the end for backwards compatibility. */
> +#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
> +#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
> +
> #ifdef __KERNEL__
> #include <linux/irqreturn.h>
> struct virtio_device;
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: jamal @ 2009-11-09 7:22 UTC (permalink / raw)
To: xiaosuo; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <4AF7B9F4.6010507@gmail.com>
On Mon, 2009-11-09 at 14:43 +0800, Changli Gao wrote:
> don't go back.
I didnt follow the motivation for the patch. Is it a bug
fix for some setup? I may be too jet-lagged and i am missing
the functional difference vs what was already there.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: David Miller @ 2009-11-09 7:26 UTC (permalink / raw)
To: xiaosuo; +Cc: netdev
In-Reply-To: <4AF7C1FD.8000302@gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 09 Nov 2009 15:17:17 +0800
> check the return value of ndo_select_queue()
>
> Check the return value of ndo_select_queue(). If the value isn't smaller
> than the real_num_tx_queues, print a warning message, and reset it to zero.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
Make it a WARN() so that it ends up in kerneloops.org
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 8:31 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257751376.8009.2.camel@bigi>
On Mon, Nov 9, 2009 at 3:22 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 14:43 +0800, Changli Gao wrote:
>> don't go back.
>
> I didnt follow the motivation for the patch. Is it a bug
> fix for some setup? I may be too jet-lagged and i am missing
> the functional difference vs what was already there.
>
code cleanup! :)
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 8:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20091108.232647.47855733.davem@davemloft.net>
On Mon, Nov 9, 2009 at 3:26 PM, David Miller <davem@davemloft.net> wrote:
> From: Changli Gao <xiaosuo@gmail.com>
> Date: Mon, 09 Nov 2009 15:17:17 +0800
>
>> check the return value of ndo_select_queue()
>>
>> Check the return value of ndo_select_queue(). If the value isn't smaller
>> than the real_num_tx_queues, print a warning message, and reset it to zero.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>
> Make it a WARN() so that it ends up in kerneloops.org
>
Like this?
WARN(1, "%s selects TX queue %d, "
"but real number of TX queues is %d\n",
dev->name, queue_index, dev->real_num_tx_queues);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: David Miller @ 2009-11-09 9:13 UTC (permalink / raw)
To: xiaosuo; +Cc: netdev
In-Reply-To: <412e6f7f0911090044v33c331edg6096c0f9b9db4143@mail.gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Mon, 9 Nov 2009 16:44:05 +0800
> On Mon, Nov 9, 2009 at 3:26 PM, David Miller <davem@davemloft.net> wrote:
>> From: Changli Gao <xiaosuo@gmail.com>
>> Date: Mon, 09 Nov 2009 15:17:17 +0800
>>
>>> check the return value of ndo_select_queue()
>>>
>>> Check the return value of ndo_select_queue(). If the value isn't smaller
>>> than the real_num_tx_queues, print a warning message, and reset it to zero.
>>>
>>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>>
>> Make it a WARN() so that it ends up in kerneloops.org
>>
> Like this?
> WARN(1, "%s selects TX queue %d, "
> "but real number of TX queues is %d\n",
> dev->name, queue_index, dev->real_num_tx_queues);
Yes, something like that.
^ permalink raw reply
* [PATCH] tcp: provide more information on the tcp receive_queue bugs
From: Ilpo Järvinen @ 2009-11-09 9:19 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, Arjan van de Ven, Netdev
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2300 bytes --]
The addition of rcv_nxt allows to discern whether the skb
was out of place or tp->copied. Also catch fancy combination
of flags if necessary (sadly we might miss the actual causer
flags as it might have already returned).
Btw, we perhaps would want to forward copied_seq in
somewhere or otherwise we might have some nice loop with
WARN stuff within but where to do that safely I don't
know at this stage until more is known (but it is not
made significantly worse by this patch).
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
net/ipv4/tcp.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 98440ad..f1813bc 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1183,7 +1183,9 @@ void tcp_cleanup_rbuf(struct sock *sk, int copied)
#if TCP_DEBUG
struct sk_buff *skb = skb_peek(&sk->sk_receive_queue);
- WARN_ON(skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq));
+ WARN(skb && !before(tp->copied_seq, TCP_SKB_CB(skb)->end_seq),
+ KERN_INFO "cleanup rbuf bug: copied %X seq %X rcvnxt %X\n",
+ tp->copied_seq, TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt);
#endif
if (inet_csk_ack_scheduled(sk)) {
@@ -1430,11 +1432,13 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
/* Now that we have two receive queues this
* shouldn't happen.
*/
- if (before(*seq, TCP_SKB_CB(skb)->seq)) {
- printk(KERN_INFO "recvmsg bug: copied %X "
- "seq %X\n", *seq, TCP_SKB_CB(skb)->seq);
+ if (WARN(before(*seq, TCP_SKB_CB(skb)->seq),
+ KERN_INFO "recvmsg bug: copied %X "
+ "seq %X rcvnxt %X fl %X\n", *seq,
+ TCP_SKB_CB(skb)->seq, tp->rcv_nxt,
+ flags))
break;
- }
+
offset = *seq - TCP_SKB_CB(skb)->seq;
if (tcp_hdr(skb)->syn)
offset--;
@@ -1443,8 +1447,9 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
if (tcp_hdr(skb)->fin)
goto found_fin_ok;
WARN(!(flags & MSG_PEEK), KERN_INFO "recvmsg bug 2: "
- "copied %X seq %X\n", *seq,
- TCP_SKB_CB(skb)->seq);
+ "copied %X seq %X rcvnxt %X fl %X\n",
+ *seq, TCP_SKB_CB(skb)->seq,
+ tp->rcv_nxt, flags);
}
/* Well, if we have backlog, try to process it now yet. */
--
1.5.6.5
^ permalink raw reply related
* [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 9:34 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, xiaosuo
check the return value of ndo_select_queue()
Check the return value of ndo_select_queue(). If the value isn't smaller
than the real_num_tx_queues, print a warning message, and reset it to zero.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
net/core/dev.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index b8f74cf..0a6bf2e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
const struct net_device_ops *ops = dev->netdev_ops;
u16 queue_index = 0;
- if (ops->ndo_select_queue)
+ if (ops->ndo_select_queue) {
queue_index = ops->ndo_select_queue(dev, skb);
- else if (dev->real_num_tx_queues > 1)
+ if (queue_index >= dev->real_num_tx_queues) {
+ WARN(1, "%s selects TX queue %d, "
+ "but real number of TX queues is %d\n",
+ dev->name, queue_index, dev->real_num_tx_queues);
+ queue_index = 0;
+ }
+ } else if (dev->real_num_tx_queues > 1) {
queue_index = skb_tx_hash(dev, skb);
+ }
skb_set_queue_mapping(skb, queue_index);
return netdev_get_tx_queue(dev, queue_index);
^ permalink raw reply related
* Re: [PATCH] check the return value of ndo_select_queue()
From: Eric Dumazet @ 2009-11-09 9:50 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev
In-Reply-To: <4AF7E221.2040901@gmail.com>
Changli Gao a écrit :
> check the return value of ndo_select_queue()
>
> Check the return value of ndo_select_queue(). If the value isn't smaller
> than the real_num_tx_queues, print a warning message, and reset it to zero.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
> net/core/dev.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b8f74cf..0a6bf2e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
> const struct net_device_ops *ops = dev->netdev_ops;
> u16 queue_index = 0;
>
> - if (ops->ndo_select_queue)
> + if (ops->ndo_select_queue) {
> queue_index = ops->ndo_select_queue(dev, skb);
> - else if (dev->real_num_tx_queues > 1)
> + if (queue_index >= dev->real_num_tx_queues) {
> + WARN(1, "%s selects TX queue %d, "
> + "but real number of TX queues is %d\n",
> + dev->name, queue_index, dev->real_num_tx_queues);
> + queue_index = 0;
> + }
> + } else if (dev->real_num_tx_queues > 1) {
> queue_index = skb_tx_hash(dev, skb);
> + }
>
> skb_set_queue_mapping(skb, queue_index);
> return netdev_get_tx_queue(dev, queue_index);
Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :
if (unlikely(queue_index >= dev->real_num_tx_queues)) {
...
}
(This unlikely() clause is implied in WARN... macros)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: Eric Dumazet @ 2009-11-09 10:02 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev
In-Reply-To: <4AF7E5F1.6080608@gmail.com>
Eric Dumazet a écrit :
>
> Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :
>
> if (unlikely(queue_index >= dev->real_num_tx_queues)) {
> ...
> }
>
> (This unlikely() clause is implied in WARN... macros)
>
Also, you want to trigger this message only once, or ratelimit it at least
^ permalink raw reply
* Re: [PATCH 2/2 net-next-2.6] au1000-eth: convert to platform_driver model
From: Florian Fainelli @ 2009-11-09 10:18 UTC (permalink / raw)
To: David Miller; +Cc: linux-mips, ralf, netdev
In-Reply-To: <20091108.210236.247950385.davem@davemloft.net>
Hi David, Ralf,
On Monday 09 November 2009 06:02:36 David Miller wrote:
> From: Florian Fainelli <florian@openwrt.org>
> Date: Sun, 8 Nov 2009 15:42:11 +0100
>
> > This patch converts the au1000-eth driver to become a full
> > platform-driver as it ought to be. We now pass PHY-speficic
> > configurations through platform_data but for compatibility
> > the driver still assumes the default settings (search for PHY1 on
> > MAC0) when no platform_data is passed. Tested on my MTX-1 board.
> >
> > Signed-off-by: Florian Fainelli <florian@openwrt.org>
>
> Ralf, feel free to merge this yourself since it depends upon
> the previous Alchemy platform patch:
>
> Acked-by: David S. Miller <davem@davemloft.net>
Thank you David. I will do a follow up patch which cleans up the driver once
that one gets merged.
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Johannes Berg @ 2009-11-09 10:22 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <4AF442C2.9040704@trash.net>
[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]
On Fri, 2009-11-06 at 16:37 +0100, Patrick McHardy wrote:
> >> This seems pretty similar to the NETLINK_URELEASE notifier invoked
> >> in netlink_release(). Wouldn't that one work as well?
> >
> > Hmm, it does seem similar, thanks for pointing it out. What exactly does
> > the condition
> > if (nlk->pid && !nlk->subscriptions) {
> >
> > mean though?
>
> nlk->pid is non-zero for bound sockets, which is basically any
> non-kernel socket which has either sent a message or explicitly
> called bind(). nlk->subscriptions is zero for sockets not bound
> to multicast groups.
>
> So effectively it invokes the notifier for all bound unicast
> userspace sockets. Not sure why it doesn't invoke the notifier
> for sockets that are used for both unicast and multicast
> reception. If that is a problem I think the second condition
> could be removed.
Thanks for the explanation. I think we'd need the second condition
removed, I don't see a reason to force a socket to not also have
multicast RX if it's used for any of the purposes we're looking at this
for. Guess we need to audit the callees to determine whether that's ok.
Can you quickly explain the difference between release and destruct?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: query: net-next section mismatch(es)
From: Andi Kleen @ 2009-11-09 10:32 UTC (permalink / raw)
To: William Allen Simpson; +Cc: David Miller, netdev
In-Reply-To: <4AF777FC.4080805@gmail.com>
William Allen Simpson <william.allen.simpson@gmail.com> writes:
>
>
> But I did it with 'make vmlinux' anyway, still makes no sense to me:
You should report those to the respective maintainers of the code that throws
the warning (see MAINTAINERS) and to linux-kernel. This is not really related
to networking.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: [PATCH 1/8] udp: add a counter into udp_hslot
From: Andi Kleen @ 2009-11-09 10:39 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Linux Netdev List, Lucian Adrian Grijincu,
Octavian Purdila
In-Reply-To: <4AF72741.5070405@gmail.com>
Eric Dumazet <eric.dumazet@gmail.com> writes:
>
> +/**
> + * struct udp_hslot - UDP hash slot
> + *
> + * @head: head of list of sockets
> + * @count: number of sockets in 'head' list
> + * @lock: spinlock protecting changes to head/count
> + */
> struct udp_hslot {
> struct hlist_nulls_head head;
> + int count;
Do you really need an int? On 64bit it's free due to the alignment,
but on 32bit x86 it's costly and you blow up the table considerably,
increasing cache misses.
Again it would be nicer if that was a separate smaller table together
with the spinlock.
In theory could also put a short counter into the low level alignment
bits of the pointer and perhaps convert the spinlock to a bitlock?
Then all could collapse into a single pointer.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: jamal @ 2009-11-09 10:54 UTC (permalink / raw)
To: Changli Gao; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <412e6f7f0911090031j16ab9c73o47f4d4d3ffd9be3a@mail.gmail.com>
On Mon, 2009-11-09 at 16:31 +0800, Changli Gao wrote:
> code cleanup! :)
I dont really see it as a cleanup to be honest. The code
is not less ugly even after the change;->
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 11:06 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257764086.6246.1.camel@bigi>
On Mon, Nov 9, 2009 at 6:54 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 16:31 +0800, Changli Gao wrote:
>
>> code cleanup! :)
>
> I dont really see it as a cleanup to be honest. The code
> is not less ugly even after the change;->
>
Maybe I should remove goto, I'll try later.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] check the return value of ndo_select_queue()
From: Changli Gao @ 2009-11-09 11:29 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev
In-Reply-To: <4AF7E8C2.90707@gmail.com>
2009/11/9 Eric Dumazet <eric.dumazet@gmail.com>:
> Eric Dumazet a écrit :
>>
>> Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :
>>
>> if (unlikely(queue_index >= dev->real_num_tx_queues)) {
>> ...
>> }
>>
>> (This unlikely() clause is implied in WARN... macros)
>>
>
> Also, you want to trigger this message only once, or ratelimit it at least
>
>
How about this version:
u16 queue_index;
u16 (*ndo_select_queue)(struct net_device*, struct sk_buff*);
unsigned int real_num_tx_queues = dev->real_num_tx_queues;
if (real_num_tx_queues == 1) {
queue_index = 0;
} else if ((ndo_select_queue = dev->netdev_ops->ndo_select_queue)) {
queue_index = ndo_select_queue(dev, skb);
if (unlikely(queue_index >= real_num_tx_queues)) {
if (net_ratelimit())
WARN(1, "%s selects TX queue %d, "
"but real number of TX queues is %d\n",
dev->name, queue_index,
real_num_tx_queues);
queue_index = 0;
}
} else {
queue_index = skb_tx_hash(dev, skb);
}
skb_set_queue_mapping(skb, queue_index);
return netdev_get_tx_queue(dev, queue_index);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: query: net-next section mismatch(es)
From: William Allen Simpson @ 2009-11-09 11:36 UTC (permalink / raw)
To: Andi Kleen; +Cc: David Miller, netdev
In-Reply-To: <87k4y056q2.fsf@basil.nowhere.org>
Andi Kleen wrote:
> You should report those to the respective maintainers of the code that throws
> the warning (see MAINTAINERS) and to linux-kernel. This is not really related
> to networking.
>
Thanks. I'll do that. I reported it here, as it was a fairly significant
change to net-next, and I assumed it was probably due to some recent problem
importing/merging some other branch into this one. But that's just a guess,
so it's phrased as a query....
^ permalink raw reply
* Re: [PATCH 1/8] udp: add a counter into udp_hslot
From: Eric Dumazet @ 2009-11-09 11:42 UTC (permalink / raw)
To: Andi Kleen
Cc: David S. Miller, Linux Netdev List, Lucian Adrian Grijincu,
Octavian Purdila
In-Reply-To: <87fx8o56eq.fsf@basil.nowhere.org>
Andi Kleen a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>> +/**
>> + * struct udp_hslot - UDP hash slot
>> + *
>> + * @head: head of list of sockets
>> + * @count: number of sockets in 'head' list
>> + * @lock: spinlock protecting changes to head/count
>> + */
>> struct udp_hslot {
>> struct hlist_nulls_head head;
>> + int count;
>
> Do you really need an int? On 64bit it's free due to the alignment,
> but on 32bit x86 it's costly and you blow up the table considerably,
> increasing cache misses.
Even a short (16 bits) might be too small for IXIACOM :)
On 32bit x86, size of hash table is 512 slots max.
(one slot per 2MB of LOWMEM, rounded to power of two)
You are speaking of <= 4096 bytes overhead :)
>
> Again it would be nicer if that was a separate smaller table together
> with the spinlock.
Nice for space, not nice for fast path, because this means additional
cache miss to get the spinlock (multicast rx still needs to take spinlock),
and some guys want really fast (low latency) multicast rx.
>
> In theory could also put a short counter into the low level alignment
> bits of the pointer and perhaps convert the spinlock to a bitlock?
> Then all could collapse into a single pointer.
>
Not enough bits in low level alignment unfortunatly. We only could give a
hint (one bit is enough) of possibly long chain, but not allowing precise
choice of shortest chain.
Once multicast is converted to RCU, then we wont need one spinlock per slot
(it wont be used in fast path, only at bind()/close() time)
and yes, we can use a separate small array to contain hashed spinlocks,
or even a single spinlock for CONFIG_BASE_SMALL :)
^ permalink raw reply
* Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-09 11:55 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911091647.29655.rusty@rustcorp.com.au>
On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote:
> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume. The only reason it's not in the ring itself
> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.
>
> I would drop avail_idx altogether: get_user is basically free, and simplifies
> a lot. As most state is in the ring, all you need is an ioctl to save/restore
> the last_avail_idx.
I remembered another reason for caching head in avail_idx. Basically,
avail index could change between when I poll for descriptors and when I
want to notify guest.
So we could have:
- poll descriptors until empty
- notify
detects not empty so does not notify
And the way to solve it would be to return flag from
notify telling us to restart the polling loop.
But, this will be more code, on data path, than
what happens today where I simply keep state
from descriptor polling and use that to notify.
I also suspect that somehow this race in practice can not create
deadlocks ... but I prefer to avoid it, these things are very tricky: if
I see an empty ring, and stop processing descriptors, I want to trigger
notify on empty.
So if we want to avoid keeping "empty" state, IMO the best way would be
to pass a flag to vhost_signal that tells it that ring is empty.
Makes sense?
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 43/75] spider-net: declare MODULE_FIRMWARE
From: Jens Osterkamp @ 2009-11-09 11:58 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, Ishizaki Kou, Jens Osterkamp, netdev, arnd
In-Reply-To: <1257630907.15927.445.camel@localhost>
On Saturday 07 November 2009, Ben Hutchings wrote:
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> drivers/net/spider_net.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/spider_net.c b/drivers/net/spider_net.c
> index 90e663f..782910c 100644
> --- a/drivers/net/spider_net.c
> +++ b/drivers/net/spider_net.c
> @@ -57,6 +57,7 @@ MODULE_AUTHOR("Utz Bacher <utz.bacher@de.ibm.com> and
> Jens Osterkamp " \ MODULE_DESCRIPTION("Spider Southbridge Gigabit Ethernet
> driver"); MODULE_LICENSE("GPL");
> MODULE_VERSION(VERSION);
> +MODULE_FIRMWARE(SPIDER_NET_FIRMWARE_NAME);
>
> static int rx_descriptors = SPIDER_NET_RX_DESCRIPTORS_DEFAULT;
> static int tx_descriptors = SPIDER_NET_TX_DESCRIPTORS_DEFAULT;
In spider_net, the filesystem is checked for a valid firmware first, if it
does not exist, the firmware is taken from the device tree (which is the
default case).
Do you expect this behaviour to cause problems together with MODULE_FIRMWARE ?
--
Best regards,
Jens Osterkamp
--------------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Erich Baier
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
^ permalink raw reply
* Re: [PATCH 1/8] udp: add a counter into udp_hslot
From: Andi Kleen @ 2009-11-09 12:10 UTC (permalink / raw)
To: Eric Dumazet
Cc: Andi Kleen, David S. Miller, Linux Netdev List,
Lucian Adrian Grijincu, Octavian Purdila
In-Reply-To: <4AF80040.6060501@gmail.com>
> > Do you really need an int? On 64bit it's free due to the alignment,
> > but on 32bit x86 it's costly and you blow up the table considerably,
> > increasing cache misses.
>
> Even a short (16 bits) might be too small for IXIACOM :)
True, but see below.
>
> On 32bit x86, size of hash table is 512 slots max.
> (one slot per 2MB of LOWMEM, rounded to power of two)
>
> You are speaking of <= 4096 bytes overhead :)
Well it's cache line overhead too. 32bit systems have often
small caches (i.e. Atom)
>
> >
> > Again it would be nicer if that was a separate smaller table together
> > with the spinlock.
>
> Nice for space, not nice for fast path, because this means additional
> cache miss to get the spinlock (multicast rx still needs to take spinlock),
> and some guys want really fast (low latency) multicast rx.
When the spinlock use is mostly local it should be in cache
(that's the nice thing about small tables, they don't drop out of cache)
>
> >
> > In theory could also put a short counter into the low level alignment
> > bits of the pointer and perhaps convert the spinlock to a bitlock?
> > Then all could collapse into a single pointer.
> >
>
> Not enough bits in low level alignment unfortunatly. We only could give a
> hint (one bit is enough) of possibly long chain, but not allowing precise
> choice of shortest chain.
Do we really need a precise answer here? I would assume an approximate
answer would be good enough using a saturating counter.
e.g. if both have >N just round robin.
Ok it would be tricky to decrement that again on unbind, but I assume just
continuing to RR later wouldn't be too bad.
The question is just if there are enough bits even for that.
> Once multicast is converted to RCU, then we wont need one spinlock per slot
> (it wont be used in fast path, only at bind()/close() time)
> and yes, we can use a separate small array to contain hashed spinlocks,
> or even a single spinlock for CONFIG_BASE_SMALL :)
Or a bit spinlock in the bucket low pointer bits.
With that (and the saturating counter) even the 64bit table could be shortened to half.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* [PATCH net-next] Phonet: put sockets in a hash table
From: Rémi Denis-Courmont @ 2009-11-09 12:17 UTC (permalink / raw)
To: netdev; +Cc: Rémi Denis-Courmont
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
include/net/phonet/phonet.h | 1 +
net/phonet/af_phonet.c | 1 +
net/phonet/socket.c | 79 +++++++++++++++++++++++++++++-------------
3 files changed, 56 insertions(+), 25 deletions(-)
diff --git a/include/net/phonet/phonet.h b/include/net/phonet/phonet.h
index fdb05fa..7b11407 100644
--- a/include/net/phonet/phonet.h
+++ b/include/net/phonet/phonet.h
@@ -46,6 +46,7 @@ static inline struct pn_sock *pn_sk(struct sock *sk)
extern const struct proto_ops phonet_dgram_ops;
+void pn_sock_init(void);
struct sock *pn_find_sock_by_sa(struct net *net, const struct sockaddr_pn *sa);
void pn_deliver_sock_broadcast(struct net *net, struct sk_buff *skb);
void phonet_get_local_port_range(int *min, int *max);
diff --git a/net/phonet/af_phonet.c b/net/phonet/af_phonet.c
index 3bd1be6..8d3a55b 100644
--- a/net/phonet/af_phonet.c
+++ b/net/phonet/af_phonet.c
@@ -481,6 +481,7 @@ static int __init phonet_init(void)
if (err)
return err;
+ pn_sock_init();
err = sock_register(&phonet_proto_family);
if (err) {
printk(KERN_ALERT
diff --git a/net/phonet/socket.c b/net/phonet/socket.c
index 0412beb..4112b6e 100644
--- a/net/phonet/socket.c
+++ b/net/phonet/socket.c
@@ -45,13 +45,28 @@ static int pn_socket_release(struct socket *sock)
return 0;
}
+#define PN_HASHSIZE 16
+#define PN_HASHMASK (PN_HASHSIZE-1)
+
+
static struct {
- struct hlist_head hlist;
+ struct hlist_head hlist[PN_HASHSIZE];
spinlock_t lock;
-} pnsocks = {
- .hlist = HLIST_HEAD_INIT,
- .lock = __SPIN_LOCK_UNLOCKED(pnsocks.lock),
-};
+} pnsocks;
+
+void __init pn_sock_init(void)
+{
+ unsigned i;
+
+ for (i = 0; i < PN_HASHSIZE; i++)
+ INIT_HLIST_HEAD(pnsocks.hlist + i);
+ spin_lock_init(&pnsocks.lock);
+}
+
+static struct hlist_head *pn_hash_list(u16 obj)
+{
+ return pnsocks.hlist + (obj & PN_HASHMASK);
+}
/*
* Find address based on socket address, match only certain fields.
@@ -64,10 +79,11 @@ struct sock *pn_find_sock_by_sa(struct net *net, const struct sockaddr_pn *spn)
struct sock *rval = NULL;
u16 obj = pn_sockaddr_get_object(spn);
u8 res = spn->spn_resource;
+ struct hlist_head *hlist = pn_hash_list(obj);
spin_lock_bh(&pnsocks.lock);
- sk_for_each(sknode, node, &pnsocks.hlist) {
+ sk_for_each(sknode, node, hlist) {
struct pn_sock *pn = pn_sk(sknode);
BUG_ON(!pn->sobject); /* unbound socket */
@@ -99,31 +115,39 @@ struct sock *pn_find_sock_by_sa(struct net *net, const struct sockaddr_pn *spn)
/* Deliver a broadcast packet (only in bottom-half) */
void pn_deliver_sock_broadcast(struct net *net, struct sk_buff *skb)
{
- struct hlist_node *node;
- struct sock *sknode;
+ struct hlist_head *hlist = pnsocks.hlist;
+ unsigned h;
spin_lock(&pnsocks.lock);
- sk_for_each(sknode, node, &pnsocks.hlist) {
- struct sk_buff *clone;
+ for (h = 0; h < PN_HASHSIZE; h++) {
+ struct hlist_node *node;
+ struct sock *sknode;
- if (!net_eq(sock_net(sknode), net))
- continue;
- if (!sock_flag(sknode, SOCK_BROADCAST))
- continue;
+ sk_for_each(sknode, node, hlist) {
+ struct sk_buff *clone;
- clone = skb_clone(skb, GFP_ATOMIC);
- if (clone) {
- sock_hold(sknode);
- sk_receive_skb(sknode, clone, 0);
+ if (!net_eq(sock_net(sknode), net))
+ continue;
+ if (!sock_flag(sknode, SOCK_BROADCAST))
+ continue;
+
+ clone = skb_clone(skb, GFP_ATOMIC);
+ if (clone) {
+ sock_hold(sknode);
+ sk_receive_skb(sknode, clone, 0);
+ }
}
+ hlist++;
}
spin_unlock(&pnsocks.lock);
}
void pn_sock_hash(struct sock *sk)
{
+ struct hlist_head *hlist = pn_hash_list(pn_sk(sk)->sobject);
+
spin_lock_bh(&pnsocks.lock);
- sk_add_node(sk, &pnsocks.hlist);
+ sk_add_node(sk, hlist);
spin_unlock_bh(&pnsocks.lock);
}
EXPORT_SYMBOL(pn_sock_hash);
@@ -439,15 +463,20 @@ EXPORT_SYMBOL(pn_sock_get_port);
static struct sock *pn_sock_get_idx(struct seq_file *seq, loff_t pos)
{
struct net *net = seq_file_net(seq);
+ struct hlist_head *hlist = pnsocks.hlist;
struct hlist_node *node;
struct sock *sknode;
+ unsigned h;
- sk_for_each(sknode, node, &pnsocks.hlist) {
- if (!net_eq(net, sock_net(sknode)))
- continue;
- if (!pos)
- return sknode;
- pos--;
+ for (h = 0; h < PN_HASHSIZE; h++) {
+ sk_for_each(sknode, node, hlist) {
+ if (!net_eq(net, sock_net(sknode)))
+ continue;
+ if (!pos)
+ return sknode;
+ pos--;
+ }
+ hlist++;
}
return NULL;
}
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-09 12:33 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257764086.6246.1.camel@bigi>
On Mon, Nov 9, 2009 at 6:54 PM, jamal <hadi@cyberus.ca> wrote:
> On Mon, 2009-11-09 at 16:31 +0800, Changli Gao wrote:
>
>> code cleanup! :)
>
> I dont really see it as a cleanup to be honest. The code
> is not less ugly even after the change;->
>
static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
struct tcf_result *res)
{
struct net_device *dev;
struct sk_buff *skb2;
u32 at;
struct tcf_mirred *m = a->priv;
int retval, err = 1;
spin_lock(&m->tcf_lock);
m->tcf_tm.lastuse = jiffies;
if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
m->tcfm_eaction != TCA_EGRESS_REDIR) {
if (net_ratelimit())
printk("tcf_mirred unknown action %d\n",
m->tcfm_eaction);
goto out;
}
dev = m->tcfm_dev;
if (!(dev->flags&IFF_UP) ) {
if (net_ratelimit())
printk("mirred to Houston: device %s is gone!\n",
dev->name);
goto out;
}
skb2 = skb_act_clone(skb, GFP_ATOMIC);
if (skb2 == NULL)
goto out;
m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
m->tcf_bstats.packets++;
at = G_TC_AT(skb->tc_verd);
if (!(at & AT_EGRESS)) {
if (m->tcfm_ok_push)
skb_push(skb2, skb2->dev->hard_header_len);
}
/* mirror is always swallowed */
if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);
skb2->dev = dev;
skb2->iif = skb->dev->ifindex;
dev_queue_xmit(skb2);
err = 0;
out:
if (err) {
m->tcf_qstats.overlimits++;
m->tcf_bstats.bytes += qdisc_pkt_len(skb);
m->tcf_bstats.packets++;
/* should we be asking for packet to be dropped?
* may make sense for redirect case only
*/
retval = TC_ACT_SHOT;
} else {
retval = m->tcf_action;
}
spin_unlock(&m->tcf_lock);
return retval;
}
How about this version.
1. move skb_act_clone() after all the necessary checks, and it can
eliminate unnecessary skb_act_clone() if tcfm_eaction isn't correct.
2. there is one exit of the critical section.
3. jump forward instead of backward.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC] netlink: add socket destruction notification
From: Patrick McHardy @ 2009-11-09 12:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev, Jouni Malinen, Thomas Graf
In-Reply-To: <1257762132.29454.161.camel@johannes.local>
Johannes Berg wrote:
> On Fri, 2009-11-06 at 16:37 +0100, Patrick McHardy wrote:
>
>>>> This seems pretty similar to the NETLINK_URELEASE notifier invoked
>>>> in netlink_release(). Wouldn't that one work as well?
>>> Hmm, it does seem similar, thanks for pointing it out. What exactly does
>>> the condition
>>> if (nlk->pid && !nlk->subscriptions) {
>>>
>>> mean though?
>> nlk->pid is non-zero for bound sockets, which is basically any
>> non-kernel socket which has either sent a message or explicitly
>> called bind(). nlk->subscriptions is zero for sockets not bound
>> to multicast groups.
>>
>> So effectively it invokes the notifier for all bound unicast
>> userspace sockets. Not sure why it doesn't invoke the notifier
>> for sockets that are used for both unicast and multicast
>> reception. If that is a problem I think the second condition
>> could be removed.
>
> Thanks for the explanation. I think we'd need the second condition
> removed, I don't see a reason to force a socket to not also have
> multicast RX if it's used for any of the purposes we're looking at this
> for. Guess we need to audit the callees to determine whether that's ok.
I've already done that. Its currently only used by netfilter
for which this change also makes sense.
> Can you quickly explain the difference between release and destruct?
release is called when the socket is closed, destruct is called
once all references are gone. I think with the synchonous processing
done nowadays they shouldn't make any difference, but release
should be fine in either case.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox