Re: [PATCH 1/4 v2] net: Introduce sk_tx_queue_mapping

Eric Dumazet [off-list ref] wrote on 10/16/2009 03:27:09 PM:

quoted

Introduce sk_tx_queue_mapping; and functions that set, test and get
this value. Reset sk_tx_queue_mapping to -1 whenever the dst cache
is set/reset, and in socket alloc & free (free probably doesn't need
it).

Could you please use an u16 instead, and take the convention of 0
being the 'unitialized value' ?

I wanted to use a signed number to avoid doing an unnecessary sub
on every iteration. I feel u16 is good for incoming path since
skb->queue_mapping is set to 0 by default, and only those drivers
doing rx recording needs to set this value. So queue_mapping reqd
the logic of using !0 to mean rx is set and the subsequent sub on
all retrieves. But on tx path where sk->txq# is saved permanently,
I feel it is not required.

If that sounds reasonable, I can change to a signed short.

And define sk_tx_queue_clear(sk) instead of sk_record_tx_queue(sk, -1);

I also suggest using following names :

static inline void sk_tx_queue_set(struct sock *sk, u16 tx_queue)
{
   sk->sk_tx_queue_mapping = tx_queue + 1;
}

static inline u16 sk_tx_queue_get(const struct sock *sk)
{
   return sk->sk_tx_queue_mapping - 1;
}

static inline u16 sk_tx_queue_clear(struct sock *sk) // or _reset
{
   sk->sk_tx_queue_mapping = 0;
}

static inline bool sk_tx_queue_recorded(const struct sock *sk)
{
   return (sk && sk->sk_tx_queue_mapping > 0);
}

quoted

@@ -1016,6 +1019,8 @@ static void sk_prot_free(struct proto *p
    struct kmem_cache *slab;
    struct module *owner;

+   sk_record_tx_queue(sk, -1);
+
    owner = prot->owner;
    slab = prot->slab;

This is not necessary, we are going to kfree(sk) anyway !

Yes, will make these changes, but please let me know your opinion on
"return sk->sk_tx_queue_mapping - 1" vs "return sk->sk_tx_queue_mapping".

Thanks,

- KK