[net-next PATCH] igb: remove skb_orphan calls

[net-next PATCH] igb: remove skb_orphan calls

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

The skb_orphan call in the tx path has been shown to cause issues as seen
with the workarounds required for timestamping.

In order to avoid this it is easiest just to remove the skb_orphan call as
the motivation for including it was purely performance based, and the
overall gain from having the call was minimal.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Acked-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 6cae258..78558f8 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3258,14 +3258,6 @@ static int igb_xmit_frame_ring_adv(struct sk_buff *skb,
 	if (unlikely(shtx->hardware)) {
 		shtx->in_progress = 1;
 		tx_flags |= IGB_TX_FLAGS_TSTAMP;
-	} else if (likely(!shtx->software)) {
-		/*
-		 * TODO: can this be solved in dev.c:dev_hard_start_xmit()?
-		 * There are probably unmodified driver which do something
-		 * like this and thus don't work in combination with
-		 * SOF_TIMESTAMPING_TX_SOFTWARE.
-		 */
-		skb_orphan(skb);
 	}
 
 	if (adapter->vlgrp && vlan_tx_tag_present(skb)) {
@@ -4253,9 +4245,6 @@ static void igb_tx_hwtstamp(struct igb_adapter *adapter, struct sk_buff *skb)
 				timecompare_transform(&adapter->compare, ns);
 			skb_tstamp_tx(skb, &shhwtstamps);
 		}
-
-		/* delayed orphaning: skb_tstamp_tx() needs the socket */
-		skb_orphan(skb);
 	}
 }
 

Re: [net-next PATCH] igb: remove skb_orphan calls

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 26 Feb 2009 02:46:23 -0800

> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> The skb_orphan call in the tx path has been shown to cause issues as seen
> with the workarounds required for timestamping.
> 
> In order to avoid this it is easiest just to remove the skb_orphan call as
> the motivation for including it was purely performance based, and the
> overall gain from having the call was minimal.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Acked-by: Mitch Williams <mitch.a.williams@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

While I'm happy to apply this, I don't see it as helping the
timestamping situation.

All someone has to do is enable the new timestamping on loopback to
trigger the problem, the loopback MUST orphan SKBs before it pushes
them back into the stack for receive.

Also, Herbert and I have talked about orphaning SKBs even earlier than
dev_queue_xmit()

This post-send timestamping scheme is not going to work, is poorly
designed, and needs to be completely rearchitected.

If it isn't fixed soon, I'll have no choice but to completely revert
all of the timestamping stuff.

Re: [net-next PATCH] igb: remove skb_orphan calls

[Jeff Kirsher]

On Thu, Feb 26, 2009 at 4:14 AM, David Miller <davem@davemloft.net> wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Thu, 26 Feb 2009 02:46:23 -0800
>
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> The skb_orphan call in the tx path has been shown to cause issues as seen
>> with the workarounds required for timestamping.
>>
>> In order to avoid this it is easiest just to remove the skb_orphan call as
>> the motivation for including it was purely performance based, and the
>> overall gain from having the call was minimal.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> Acked-by: Mitch Williams <mitch.a.williams@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> While I'm happy to apply this, I don't see it as helping the
> timestamping situation.
>
> All someone has to do is enable the new timestamping on loopback to
> trigger the problem, the loopback MUST orphan SKBs before it pushes
> them back into the stack for receive.
>
> Also, Herbert and I have talked about orphaning SKBs even earlier than
> dev_queue_xmit()
>
> This post-send timestamping scheme is not going to work, is poorly
> designed, and needs to be completely rearchitected.
>
> If it isn't fixed soon, I'll have no choice but to completely revert
> all of the timestamping stuff.
> --
>

Adding Emil and Patrick.

While no issues were seen in testing, I will verify with Emil that
loopback testing was done.  Also I will work with Patrick to ensure
that we resolve any issues with lookback and timestamping.

-- 
Cheers,
Jeff

RE: [net-next PATCH] igb: remove skb_orphan calls

[Duyck, Alexander H]

Jeff Kirsher wrote:
> On Thu, Feb 26, 2009 at 4:14 AM, David Miller <davem@davemloft.net>
> wrote: 
>> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Date: Thu, 26 Feb 2009 02:46:23 -0800
>> 
>>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>> 
>>> The skb_orphan call in the tx path has been shown to cause issues
>>> as seen with the workarounds required for timestamping.
>>> 
>>> In order to avoid this it is easiest just to remove the skb_orphan
>>> call as the motivation for including it was purely performance
>>> based, and the overall gain from having the call was minimal.
>>> 
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>> Acked-by: Mitch Williams <mitch.a.williams@intel.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> 
>> While I'm happy to apply this, I don't see it as helping the
>> timestamping situation. 
>> 
>> All someone has to do is enable the new timestamping on loopback to
>> trigger the problem, the loopback MUST orphan SKBs before it pushes
>> them back into the stack for receive.
>> 
>> Also, Herbert and I have talked about orphaning SKBs even earlier
>> than dev_queue_xmit() 
>> 
>> This post-send timestamping scheme is not going to work, is poorly
>> designed, and needs to be completely rearchitected.
>> 
>> If it isn't fixed soon, I'll have no choice but to completely revert
>> all of the timestamping stuff.
>> --
>> 
> 
> Adding Emil and Patrick.
> 
> While no issues were seen in testing, I will verify with Emil that
> loopback testing was done.  Also I will work with Patrick to ensure
> that we resolve any issues with lookback and timestamping.

This patch was meant to be more general than just timestamping.  I was just using timestamping as one example of where things can go wrong due to the skb_orphan call.  The main problem is triggering desctructors and removing the sk can cause multiple issues with any custom use of the skb.  It is easiest to avoid those by not calling skb_orphan in the first place.

Thanks,

Alex

Re: [net-next PATCH] igb: remove skb_orphan calls

[Herbert Xu]

Duyck, Alexander H <alexander.h.duyck@intel.com> wrote:
>
> This patch was meant to be more general than just timestamping.  I was just using timestamping as one example of where things can go wrong due to the skb_orphan call.  The main problem is triggering desctructors and removing the sk can cause multiple issues with any custom use of the skb.  It is easiest to avoid those by not calling skb_orphan in the first place.

It doesn't matter what those custom uses are, what Dave was saying
is that we're thinking of killing skb->sk before it even gets to
the driver.  So even if such custom uses existed (which AFAIK do
not), we'd been looking at killing them as well.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [net-next PATCH] igb: remove skb_orphan calls

[Duyck, Alexander H]

Herbert Xu wrote:
> Duyck, Alexander H <alexander.h.duyck@intel.com> wrote:
>> 
>> This patch was meant to be more general than just timestamping.  I
>> was just using timestamping as one example of where things can go
>> wrong due to the skb_orphan call.  The main problem is triggering
>> desctructors and removing the sk can cause multiple issues with any
>> custom use of the skb.  It is easiest to avoid those by not calling
>> skb_orphan in the first place.     
> 
> It doesn't matter what those custom uses are, what Dave was saying
> is that we're thinking of killing skb->sk before it even gets to
> the driver.  So even if such custom uses existed (which AFAIK do
> not), we'd been looking at killing them as well.

The fact is, if you are planning to kill it further up the stack, you can put that down as another reason for removing the call.  The timesync / killing skb->sk is a separate discussion that I don't know if I even really need to be involved in.  The patch doesn't break anything, and removes a call to skb_orphan that shouldn't really be there in the first place.

Thanks,

Alex

Re: [net-next PATCH] igb: remove skb_orphan calls

[David Miller]

From: "Duyck, Alexander H" <alexander.h.duyck@intel.com>
Date: Fri, 27 Feb 2009 08:39:30 -0800

> The fact is, if you are planning to kill it further up the stack,
> you can put that down as another reason for removing the call.  The
> timesync / killing skb->sk is a separate discussion that I don't
> know if I even really need to be involved in.  The patch doesn't
> break anything, and removes a call to skb_orphan that shouldn't
> really be there in the first place.

That's not true at all.

One reason we haven't removed it yet is because there are some
semantic issues to address, which are also effected by drivers
doing this kind of by-hand skb_orphan()'ing.

For one thing, if you skb_orphan() in your transmit handler,
a UDP application can take over your network card by just
transmitting in an endless tight loop.  It would be able to
nearly keep all other users from using the card.

That happens because if you skb_orphan(), nothing controls the
pace at which datagram applications can send.  Currently
we need the socket send buffer limits to help us out here,
but if you skb_orphan() you completely subvert that proptection.

So I will not apply this patch.

Re: [net-next PATCH] igb: remove skb_orphan calls

[Jeff Kirsher]

On Fri, Feb 27, 2009 at 4:10 PM, David Miller <davem@davemloft.net> wrote:
> From: "Duyck, Alexander H" <alexander.h.duyck@intel.com>
> Date: Fri, 27 Feb 2009 08:39:30 -0800
>
>> The fact is, if you are planning to kill it further up the stack,
>> you can put that down as another reason for removing the call.  The
>> timesync / killing skb->sk is a separate discussion that I don't
>> know if I even really need to be involved in.  The patch doesn't
>> break anything, and removes a call to skb_orphan that shouldn't
>> really be there in the first place.
>
> That's not true at all.
>
> One reason we haven't removed it yet is because there are some
> semantic issues to address, which are also effected by drivers
> doing this kind of by-hand skb_orphan()'ing.
>
> For one thing, if you skb_orphan() in your transmit handler,
> a UDP application can take over your network card by just
> transmitting in an endless tight loop.  It would be able to
> nearly keep all other users from using the card.
>
> That happens because if you skb_orphan(), nothing controls the
> pace at which datagram applications can send.  Currently
> we need the socket send buffer limits to help us out here,
> but if you skb_orphan() you completely subvert that proptection.
>
> So I will not apply this patch.
> --

So the removal of the skb_orphan() in the driver should be a good
thing, the only issue is with the patch description?  By cleaning up
the patch description, will that make this patch acceptable for
everyone?

-- 
Cheers,
Jeff

Re: [net-next PATCH] igb: remove skb_orphan calls

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 27 Feb 2009 17:02:48 -0800

> So the removal of the skb_orphan() in the driver should be a good
> thing, the only issue is with the patch description?  By cleaning up
> the patch description, will that make this patch acceptable for
> everyone?

Yes, that would work for me, thanks Jeff.