Re: [GIT]: Networking

Linus Torvalds
2009-06-16T19:00:03Z

On Tue, 16 Jun 2009, Eric Dumazet wrote:
>
> Here is first patch to take into account this initial offset in sk_wmem_alloc
>
> (Only compiled, not tested)

I think this is incredibly ugly and hacky.

> @@ -162,7 +162,7 @@ static void atalk_destroy_timer(unsigned long data)
> {
> struct sock *sk = (struct sock *)data;
>
> - if (atomic_read(&sk->sk_wmem_alloc) ||
> + if (atomic_read(&sk->sk_wmem_alloc) > 1 ||
> atomic_read(&sk->sk_rmem_alloc)) {

Seriously, look at that code, and tell me it makes sense.

No, it does not. The code looks like totally random line noise, and that
whole "> 1" test makes no conceptual sense what-so-ever.

It _will_ result in random bugs later on, because code that doesn't make
sense will never be good in the long run.

At the very least, add a helper function for "do I actually have
outstanding allocations" or something like that. IOW, do a

/*
* Comment here about that magical "1"
*/
static inline int sk_has_allocations(struct sock *sk)
{
return atomic_read(&sk->sk_wmem_alloc) > 1 ||
atomic_read(&sk->sk_rmem_alloc);
}

and then make the various network protocols use that, rather than
open-coding some random internal implementation magic.

Linus