Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
Jesper Juhl wrote: > This moves the updating of both tx_bytes and tx_packets inside the > spinlock, but as far as I can see we only _really_ need to move the > tx_bytes update. Considering that we generally want to do as little > work as possible while holding a lock, wouldn't the following be > slightly better? > Hm, I think it would be better to keep them together. The second add is going to be pretty much free, particularly since the tx_bytes add will probably pull tx_packets into cache. I have a followup patch to convert it to using the netdevice stats structure, which will definitely put them in the same cacheline (though perhaps the stats structure should group tx and rx members together?). J J - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
Jesper Juhl wrote: This moves the updating of both tx_bytes and tx_packets inside the spinlock, but as far as I can see we only _really_ need to move the tx_bytes update. Considering that we generally want to do as little work as possible while holding a lock, wouldn't the following be slightly better? Hm, I think it would be better to keep them together. The second add is going to be pretty much free, particularly since the tx_bytes add will probably pull tx_packets into cache. I have a followup patch to convert it to using the netdevice stats structure, which will definitely put them in the same cacheline (though perhaps the stats structure should group tx and rx members together?). J J - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
Jeremy Fitzhardinge wrote: xennet_tx_bug_gc can free the skb before we use it, so make sure we don't. Jeff, this is -rc material. Signed-off-by: Keir Fraser <[EMAIL PROTECTED]> Signed-off-by: Jeremy Fitzhardinge <[EMAIL PROTECTED]> Cc: Jeff Garzik <[EMAIL PROTECTED]> applied - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
On Monday 13 August 2007 21:54:37 Jeremy Fitzhardinge wrote: > xennet_tx_bug_gc can free the skb before we use it, so make sure we don't. > > Jeff, this is -rc material. > > Signed-off-by: Keir Fraser <[EMAIL PROTECTED]> > Signed-off-by: Jeremy Fitzhardinge <[EMAIL PROTECTED]> > Cc: Jeff Garzik <[EMAIL PROTECTED]> > > diff -r 8bfc43f6d1b0 drivers/net/xen-netfront.c > --- a/drivers/net/xen-netfront.c Tue Aug 07 14:26:30 2007 -0700 > +++ b/drivers/net/xen-netfront.c Mon Aug 13 09:39:15 2007 -0700 > @@ -566,15 +566,16 @@ static int xennet_start_xmit(struct sk_b > if (notify) > notify_remote_via_irq(np->netdev->irq); > > + np->stats.tx_bytes += skb->len; > + np->stats.tx_packets++; > + > + /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */ > xennet_tx_buf_gc(dev); > > if (!netfront_tx_slot_available(np)) > netif_stop_queue(dev); > > spin_unlock_irq(>tx_lock); > - > - np->stats.tx_bytes += skb->len; > - np->stats.tx_packets++; > > return 0; > This moves the updating of both tx_bytes and tx_packets inside the spinlock, but as far as I can see we only _really_ need to move the tx_bytes update. Considering that we generally want to do as little work as possible while holding a lock, wouldn't the following be slightly better? Signed-off-by: Keir Fraser <[EMAIL PROTECTED]> Signed-off-by: Jeremy Fitzhardinge <[EMAIL PROTECTED]> Cc: Jeff Garzik <[EMAIL PROTECTED]> Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]> --- drivers/net/xen-netfront.c |4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 489f69c..640e270 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -566,6 +566,9 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) if (notify) notify_remote_via_irq(np->netdev->irq); + np->stats.tx_bytes += skb->len; + + /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */ xennet_tx_buf_gc(dev); if (!netfront_tx_slot_available(np)) @@ -573,7 +576,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) spin_unlock_irq(>tx_lock); - np->stats.tx_bytes += skb->len; np->stats.tx_packets++; return 0; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
xennet_tx_bug_gc can free the skb before we use it, so make sure we don't. Jeff, this is -rc material. Signed-off-by: Keir Fraser <[EMAIL PROTECTED]> Signed-off-by: Jeremy Fitzhardinge <[EMAIL PROTECTED]> Cc: Jeff Garzik <[EMAIL PROTECTED]> diff -r 8bfc43f6d1b0 drivers/net/xen-netfront.c --- a/drivers/net/xen-netfront.cTue Aug 07 14:26:30 2007 -0700 +++ b/drivers/net/xen-netfront.cMon Aug 13 09:39:15 2007 -0700 @@ -566,15 +566,16 @@ static int xennet_start_xmit(struct sk_b if (notify) notify_remote_via_irq(np->netdev->irq); + np->stats.tx_bytes += skb->len; + np->stats.tx_packets++; + + /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */ xennet_tx_buf_gc(dev); if (!netfront_tx_slot_available(np)) netif_stop_queue(dev); spin_unlock_irq(>tx_lock); - - np->stats.tx_bytes += skb->len; - np->stats.tx_packets++; return 0; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
xennet_tx_bug_gc can free the skb before we use it, so make sure we don't. Jeff, this is -rc material. Signed-off-by: Keir Fraser [EMAIL PROTECTED] Signed-off-by: Jeremy Fitzhardinge [EMAIL PROTECTED] Cc: Jeff Garzik [EMAIL PROTECTED] diff -r 8bfc43f6d1b0 drivers/net/xen-netfront.c --- a/drivers/net/xen-netfront.cTue Aug 07 14:26:30 2007 -0700 +++ b/drivers/net/xen-netfront.cMon Aug 13 09:39:15 2007 -0700 @@ -566,15 +566,16 @@ static int xennet_start_xmit(struct sk_b if (notify) notify_remote_via_irq(np-netdev-irq); + np-stats.tx_bytes += skb-len; + np-stats.tx_packets++; + + /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */ xennet_tx_buf_gc(dev); if (!netfront_tx_slot_available(np)) netif_stop_queue(dev); spin_unlock_irq(np-tx_lock); - - np-stats.tx_bytes += skb-len; - np-stats.tx_packets++; return 0; - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
On Monday 13 August 2007 21:54:37 Jeremy Fitzhardinge wrote: xennet_tx_bug_gc can free the skb before we use it, so make sure we don't. Jeff, this is -rc material. Signed-off-by: Keir Fraser [EMAIL PROTECTED] Signed-off-by: Jeremy Fitzhardinge [EMAIL PROTECTED] Cc: Jeff Garzik [EMAIL PROTECTED] diff -r 8bfc43f6d1b0 drivers/net/xen-netfront.c --- a/drivers/net/xen-netfront.c Tue Aug 07 14:26:30 2007 -0700 +++ b/drivers/net/xen-netfront.c Mon Aug 13 09:39:15 2007 -0700 @@ -566,15 +566,16 @@ static int xennet_start_xmit(struct sk_b if (notify) notify_remote_via_irq(np-netdev-irq); + np-stats.tx_bytes += skb-len; + np-stats.tx_packets++; + + /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */ xennet_tx_buf_gc(dev); if (!netfront_tx_slot_available(np)) netif_stop_queue(dev); spin_unlock_irq(np-tx_lock); - - np-stats.tx_bytes += skb-len; - np-stats.tx_packets++; return 0; This moves the updating of both tx_bytes and tx_packets inside the spinlock, but as far as I can see we only _really_ need to move the tx_bytes update. Considering that we generally want to do as little work as possible while holding a lock, wouldn't the following be slightly better? Signed-off-by: Keir Fraser [EMAIL PROTECTED] Signed-off-by: Jeremy Fitzhardinge [EMAIL PROTECTED] Cc: Jeff Garzik [EMAIL PROTECTED] Signed-off-by: Jesper Juhl [EMAIL PROTECTED] --- drivers/net/xen-netfront.c |4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 489f69c..640e270 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -566,6 +566,9 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) if (notify) notify_remote_via_irq(np-netdev-irq); + np-stats.tx_bytes += skb-len; + + /* Note: It is not safe to access skb after xennet_tx_buf_gc()! */ xennet_tx_buf_gc(dev); if (!netfront_tx_slot_available(np)) @@ -573,7 +576,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) spin_unlock_irq(np-tx_lock); - np-stats.tx_bytes += skb-len; np-stats.tx_packets++; return 0; - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.
Jeremy Fitzhardinge wrote: xennet_tx_bug_gc can free the skb before we use it, so make sure we don't. Jeff, this is -rc material. Signed-off-by: Keir Fraser [EMAIL PROTECTED] Signed-off-by: Jeremy Fitzhardinge [EMAIL PROTECTED] Cc: Jeff Garzik [EMAIL PROTECTED] applied - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
