3 ms·

if (rmem > (size + sk->sk_rcvbuf)) goto uncharge_drop; What is rmem in this case? I'm a bit confused as to why it is written that way. This drops the pac

• jeffbee 6y ago
  It's not very literate, is it? rmem is initially the sk_backlog.rmem_alloc field of struct sock. There is no comment in net/sock.h what this field might mean. People who modify this function just have to guess. I also appreciate that this function adds |size| to rmem_alloc, tests for limits, then later it subtracts |truesize| from rmem_alloc. This happens to seem correct, but it's just asking for someone to accidentally screw up the accounting in a later change. Reading this function only reinforces my view of Linux code quality.
  • jeffbee 6y ago
    Another interesting thing is how stewardship of this logic and the comment right above it have diverged. Original: /* we drop only if the receive buf is full and the receive * queue contains some other skb */ rmem = atomic_add_return(size, &sk->sk_rmem_alloc); if ((rmem > sk->sk_rcvbuf) && (rmem > size)) goto uncharge_drop; Later: /* we drop only if the receive buf is full and the receive * queue contains some other skb */ rmem = atomic_add_return(size, &sk->sk_rmem_alloc); if (rmem > (size + sk->sk_rcvbuf)) goto uncharge_drop; How does the comment correspond to each block?
• crdrost 6y ago
  Basically rmem - size was the number of bytes that were consumed before this current packet. In the line before, we have thread-atomically added size to rmem immediately prior and read the current value in one single step. Call this "staking our claim" to part of the buffer, and the meaning of "uncharge" here is discharging this claim by atomically decrementing the counter, before dropping the packet. Probably this bug would not have happened if this comparison were written as `rmem - size > sk->sk_rcvbuf`? So it is saying that the simplest sanity check is "if the buffer was already full before we staked our claim we should drop this packet immediately." As the "goto" indicates, there are then a bunch more checks on other circumstances where we should also drop the packet. Due to the quirks of multi-threading it is of course possible that some packets get unnecessarily dropped between when we stake the claim and when we discharge it, which the code just accepts -- the thinking is presumably "yeah if the buffer is full a lot of packets are gonna get dropped and that's just life -- it's much less important that we dropped some extra packets when we were already dropping packets, and much more important that we don't mismanage the buffer's memory when it's nearly full." A comment suggests that part of the reason for this awkward phrasing is that it is possible for rmem = size, in other words the buffer was empty when we staked our claim--and in this case we don't want to drop this packet even if it would overflow a small max-buffer-size. I think the idea there is "we already have the socket buffer allocated, obviously this thing fits in memory, so let's just handle it if the queue is empty rather than dropping every single packet that is larger than the queue size."