| Message ID | 20140910200442.15961.84466.stgit@ahduyck-bv4.jf.intel.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2014-09-10 at 16:06 -0400, Alexander Duyck wrote: > The code for cloning the skb for an acknowledgement was checking to see if > the cloned skb was shared and if it was it was then freeing the original > skb. Since a clone should never really be shared I suspect that the > intention was to avoid freeing the clone if the original was shared. As > such I am updating the code so that if the original is shared we free the > original and use the clone. This avoids unnecessary work in the next > section where we would be cloning the skb if the original is shared. Thanks, yeah, I admit that this is clearly fishy. > @@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, > if (id >= 0) { > info_id = id; > info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; Luckily, we practically always go into this path. > - } else if (skb_shared(skb)) { > + } else if (skb_shared(orig_skb)) { > kfree_skb(orig_skb); > } else { > kfree_skb(skb); We have a clone already so we could just remove the whole "else if" I think, but I'm guessing my intent was to keep it accounted to the socket where possible rather than freeing the original in all cases. So yeah, I think this makes sense. Maybe we should add a comment to the if though to explain this? johannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/2014 01:33 PM, Johannes Berg wrote: > On Wed, 2014-09-10 at 16:06 -0400, Alexander Duyck wrote: >> The code for cloning the skb for an acknowledgement was checking to see if >> the cloned skb was shared and if it was it was then freeing the original >> skb. Since a clone should never really be shared I suspect that the >> intention was to avoid freeing the clone if the original was shared. As >> such I am updating the code so that if the original is shared we free the >> original and use the clone. This avoids unnecessary work in the next >> section where we would be cloning the skb if the original is shared. > > Thanks, yeah, I admit that this is clearly fishy. > >> @@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, >> if (id >= 0) { >> info_id = id; >> info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; > > Luckily, we practically always go into this path. > >> - } else if (skb_shared(skb)) { >> + } else if (skb_shared(orig_skb)) { >> kfree_skb(orig_skb); >> } else { >> kfree_skb(skb); > > We have a clone already so we could just remove the whole "else if" I > think, but I'm guessing my intent was to keep it accounted to the socket > where possible rather than freeing the original in all cases. > > So yeah, I think this makes sense. Maybe we should add a comment to the > if though to explain this? > > johannes Actually I think we may need to take a different approach. The reason I was in this code was to take a look at a possible refcount issue. I'll be submitting another patch in a few minutes and will probably be dropping some of this code anyway. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 925c39f..e527cd3 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2087,7 +2087,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb, if (id >= 0) { info_id = id; info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS; - } else if (skb_shared(skb)) { + } else if (skb_shared(orig_skb)) { kfree_skb(orig_skb); } else { kfree_skb(skb);
The code for cloning the skb for an acknowledgement was checking to see if the cloned skb was shared and if it was it was then freeing the original skb. Since a clone should never really be shared I suspect that the intention was to avoid freeing the clone if the original was shared. As such I am updating the code so that if the original is shared we free the original and use the clone. This avoids unnecessary work in the next section where we would be cloning the skb if the original is shared. Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> --- net/mac80211/tx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html