netdev - Re: [PATCH 1/4 v2] net: Introduce sk_tx_queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OF57EE3C38.CD9A83EC-ON65257651.00405FE1-65257651.0047491F@in.ibm.com>
Date:	Fri, 16 Oct 2009 18:28:37 +0530
From:	Krishna Kumar2 <krkumar2@...ibm.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	davem@...emloft.net, herbert@...dor.apana.org.au,
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/4 v2] net: Introduce sk_tx_queue_mapping

Eric Dumazet <eric.dumazet@...il.com> wrote on 10/16/2009 03:27:09 PM:

>
> > 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);
> }
> >
> > @@ -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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html