On Tue, 2012-10-30 at 20:52 +0100, Krzysztof Mazur wrote: > > --- a/net/atm/pppoatm.c > +++ b/net/atm/pppoatm.c > @@ -306,12 +306,9 @@ static int pppoatm_send(struct ppp_channel *chan, struct > sk_buff *skb) > > /* > * It's not clear that we need to bother with using atm_may_send() > - * to check we don't exceed sk->sk_sndbuf. If userspace sets a > - * value of sk_sndbuf which is lower than the MTU, we're going to > - * block for ever. But the code always did that before we introduced > - * the packet count limit, so... > + * to check we don't exceed sk->sk_sndbuf. > */ > - if (!atm_may_send(vcc, skb->truesize)) > + if (sk_wmem_alloc_get(sk_atm(vcc)) && !atm_may_send(vcc, > skb->truesize)) > goto nospace_unlock_sock;
Does this break the pvcc->blocked handling that coordinates with pppoatm_pop()? If we have one packet in flight, so pppoatm_may_send() permits a new one to be queued... but they're *large* packets to sk_wmem_alloc doesn't permit it. Immediately after the check, pppoatm_pop() runs and leaves the queue empty. We return zero, blocking the queue… which never gets woken because we didn't set the BLOCKED flag and thus the tasklet never runs. In fact, I think we need the BLOCKED handling for the sock_owned_by_user() case too? When the VCC is actually closed, I suppose that's not recoverable and we don't care about waking the queue anyway? But any time we end up returning zero from pppoatm_send(), we *need* to ensure that a wakeup will happen in future unless the socket is actually dead. -- dwmw2
smime.p7s
Description: S/MIME cryptographic signature