net/context: Unlock the context while allocating a net_pkt #14670
Conversation
But reference it once more to make sure it will not disappear during packet allocation. That way, the lock is not going to be held during net_pkt allocation. Fixes zephyrproject-rtos#14571 Signed-off-by: Tomasz Bursztyka <[email protected]>
|
@dgloeck have a look |
|
There are many places where prepare_segment is used without the context to be locked before, in tcp. Only one place using send_ack() was, for some reason. Sounds fragile. |
| struct net_pkt *pkt; | ||
|
|
||
| /* We reference it once again to make sure context is not going | ||
| * to disappear while allocating a packet |
There was a problem hiding this comment.
What would make it disappear?
There was a problem hiding this comment.
is there any guaranty that net_context_unref() is not going to be called one time too many (or net_context_put) in the mean time? allocation might take a while depending on load. The application could close the socket (thus the context) for instance.
There was a problem hiding this comment.
The application could close the socket (thus the context) for instance.
Right. And as I mentioned in the next comment (in the main comment thread), this touches on other (proposed) changes, e.g. #9565 , which vice-versa try to remove ref/unref wrapping, about which I came to conclusion that it's superfluous. Is it right or not? I wish we had a clear rules and understanding where we need to lock/unlock, and where ref/unref. As certainly, a case addressed by this PR is not the only one.
And - why is that important, what does it give? Please don't just stamp "Fixes #14571", describe the issue in the commit message. Otherwise, this touches on #8008, #13667, #9565 , and I'm not sure if it goes in the same direction as those, against them, or largely orthogonal (I would hope and guess for the latter). |
|
@dgloeck, @tbursztyka: I believe the situation with this patch is as follows: This patch is an alternative to a patch originally proposed by @dgloeck (reference to which I can't even find now). So, I would say that other reviewer wait for the discussion between you guys, whose patch better addresses this issue. Hope you can get to it soon, as the issue appears to be serious and worth fixing. Thanks. |
|
@tbursztyka: Btw, this needs rebase to fix conflict. |
Ah, ok, @dgloeck didn't submit his patch as a PR, it's available at dgloeck@e61634c (based on comment #14571 (comment)) |
|
replaced by #14944 |
3 participants
But reference it once more to make sure it will not disappear during
packet allocation.
That way, the lock is not going to be held during net_pkt allocation.
Fixes #14571
Signed-off-by: Tomasz Bursztyka [email protected]