On Sat, Dec 15, 2007 at 12:15:04AM -0500, Hideo AOKI wrote:
> This patch includes changes in network core sub system for memory
> accounting.
>
> Memory scheduling, charging, uncharging and reclaiming functions are
> added. These functions use sk_forward_alloc to store socket local
> accounting. They also need to use lock to keep consistency of
> sk_forward_alloc and memory_allocated. They currently support only
> datagram protocols.
Thanks for the patch. I think it's generally on the right track
but there's still a few issues with the implementation.
> + spin_lock_irqsave(&sk->sk_lock.slock, flags);
Please use bh_lock_sock since this must never be used from an
IRQ handler.
> +static inline void sk_mem_reclaim(struct sock *sk)
> +{
> + if (sk->sk_type == SOCK_DGRAM)
> + sk_datagram_mem_reclaim(sk);
> +}
Please get rid of these wrappers since we should only get here
for datagram protocols.
> +static inline int sk_account_wmem_charge(struct sock *sk, int size)
> +{
> + unsigned long flags;
> +
> + /* account if protocol supports memory accounting. */
> + if (!sk->sk_prot->memory_allocated || sk->sk_type != SOCK_DGRAM)
> + return 1;
> +
> + spin_lock_irqsave(&sk->sk_lock.slock, flags);
> + if (sk_datagram_wmem_schedule(sk, size)) {
> + sk->sk_forward_alloc -= size;
> + spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
> + return 1;
> + }
> + spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
> + return 0;
> +}
This is probably too big to inline.
> +static inline int sk_account_rmem_charge(struct sock *sk, int size)
> +{
> + unsigned long flags;
> +
> + /* account if protocol supports memory accounting. */
> + if (!sk->sk_prot->memory_allocated || sk->sk_type != SOCK_DGRAM)
> + return 1;
> +
> + spin_lock_irqsave(&sk->sk_lock.slock, flags);
> + if (sk_datagram_rmem_schedule(sk, size)) {
> + sk->sk_forward_alloc -= size;
> + spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
> + return 1;
> + }
> + spin_unlock_irqrestore(&sk->sk_lock.slock, flags);
> + return 0;
> +}
Why are we duplicating the rmem/wmem versions when they're identical?
> -int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> +int sock_queue_rcv_skb_with_owner(struct sock *sk, struct sk_buff *skb,
> + void set_owner_r(struct sk_buff *nskb,
> + struct sock* nsk))
Just make a new function for this rather than playing with function
pointers.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
