Re: [net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short

On Mon, Jun 25, 2018 at 6:34 PM, Tom Herbert [off-list ref] wrote:

On Mon, Jun 25, 2018 at 11:04 AM, Amritha Nambiar
[off-list ref] wrote:

quoted

Change 'skc_tx_queue_mapping' field in sock_common structure from
'int' to 'unsigned short' type with 0 indicating unset and
a positive queue value being set. This way it is consistent with
the queue_mapping field in the sk_buff. This will also accommodate
adding a new 'unsigned short' field in sock_common in the next
patch for rx_queue_mapping.

Signed-off-by: Amritha Nambiar <redacted>
---
 include/net/sock.h |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index b3b7541..009fd30 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

@@ -214,7 +214,7 @@ struct sock_common {
                struct hlist_node       skc_node;
                struct hlist_nulls_node skc_nulls_node;
        };
-       int                     skc_tx_queue_mapping;
+       unsigned short          skc_tx_queue_mapping;
        union {
                int             skc_incoming_cpu;
                u32             skc_rcv_wnd;

@@ -1681,17 +1681,19 @@ static inline int sk_receive_skb(struct sock *sk,

struct sk_buff *skb,

 static inline void sk_tx_queue_set(struct sock *sk, int tx_queue)
 {
-       sk->sk_tx_queue_mapping = tx_queue;
+       /* sk_tx_queue_mapping accept only upto a 16-bit value */
+       WARN_ON((unsigned short)tx_queue > USHRT_MAX);

Shouldn't this be USHRT_MAX - 1 ?

Actually just a ">=" would probably do as well.

quoted

+       sk->sk_tx_queue_mapping = tx_queue + 1;
 }

 static inline void sk_tx_queue_clear(struct sock *sk)
 {
-       sk->sk_tx_queue_mapping = -1;

+       sk->sk_tx_queue_mapping = 0;

I think it's slightly better to define a new constant like NO_QUEUE_MAPPING
to be USHRT_MAX. That avoids needing to do the arithmetic every time the
value is accessed.

quoted

 }

 static inline int sk_tx_queue_get(const struct sock *sk)
 {
-       return sk ? sk->sk_tx_queue_mapping : -1;
+       return sk ? sk->sk_tx_queue_mapping - 1 : -1;

Doesn't the comparison in __netdev_pick_tx need to be simultaneously changed
for this?

This doesn't change the result. It was still -1 if the queue mapping
is not set. It was just initialized to 0 instead of to -1 so we have
to perform the operation to get there.

Also in regards to the comment above about needing an extra operation
I am not sure it makes much difference.

In the case of us starting with 0 as a reserved value I think the
instruction count should be about the same. We move the unsigned short
into an unsigned in, then decrement, and if the value is non-negative
we can assume it is valid. Although maybe I should double check the
code to make certain it is doing what I thought it was supposed to be
doing.

quoted

 }

 static inline void sk_set_socket(struct sock *sk, struct socket *sock)