On Thu, 2014-09-11 at 08:21 -0700, Alexander Duyck wrote: [...] > >> EXPORT_SYMBOL_GPL(skb_complete_wifi_ack); > > > > Here I'm not sure it matters *for this function*? Wouldn't it be freed > > then in sock_put(), which has the same net effect on this function > > overall? It doesn't use it after sock_queue_err_skb(). > > The significant piece is that we are calling sock_put *after*. So if we > are dropping the last reference the buffer is already in the > sk_error_queue and will be purged when __sk_free is called. Yeah, I understand. But that's more of a problem of sock_queue_err_skb() rather than this function. > > Seems like maybe this should be in sock_queue_err_skb() itself, since it > > does the orphaning first and then looks at the socket. Or the > > documentation for that function should state that it has to be held, but > > there are plenty of callers? > > The problem is there are a number of cases where the sock_hold/put are > not needed. For example, if we were to clone the skb and immediately > send the clone up the sk_error_queue then we don't need it. We only > need it if there is a risk that orphaning the buffer sent could > potentially result in the destructor calling __sk_free. Ok, that's reasonable. Maybe then you can add that to the documentation of sock_queue_err_skb() - that it must (somehow) ensure the socket can't go away while it's being called? That way this caller change would become clearer IMHO. > > So you're removing this part, but can't we really not reuse the clone_sk > > copy? The difference is that it's charged, but that's fine for the > > purposes here, no? Or am I misunderstanding that? > The copy being held cannot really be used for transmit. The problem is > that it is holding the wrong kind of reference. Ok. > The problem lies in the order things are released. The sock_put > function will dec_and_test sk_refcnt, once it reaches 0 it will do a > dec_and_test on sk_wmem_alloc to see if it should call __sk_free. Until > that reaches 0 sk_wmem_alloc cannot reach 0. Once either of these drops > to 0 we cannot bring the value back up from there. So if I were to > transmit the clone then it could let the sk_refcnt drop to 0 in which > case any calls to sock_hold are invalid. > > I would need to somehow hold the reference based on sk_wmem_alloc if we > want to transmit the clone. Many of the hardware timestamping drivers > seem to just clone the original skb, queue that clone onto the > sk_error_queue, and then free the original after completing the call. I > suppose we could change it to something like that, but you are still > looking at possibly 2 clones in that case anyway. Well, no need. I just had originally wanted to reuse the clone so under these corner case conditions we didn't clone twice - no big deal, it never happens anyway (that IDR thing should never actually run out of space) Anyway, thanks. I expect due to the patch 1 davem will apply both patches (and I'm going to be on vacation anyway), so Acked-by: Johannes Berg <johannes@xxxxxxxxxxxxxxxx> for both patches. Thanks! johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
