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)
On 6/25/2018 8:25 PM, Alexander Duyck wrote:
On Mon, Jun 25, 2018 at 6:34 PM, Tom Herbert [off-list ref] wrote:
quoted
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.
Ugh! Will definitely fix this.
quoted
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.
The idea was to have avoid having to make any changes to the callers of
these functions and make this similar to queue_mapping in skbuff with 0
indicating unset and +ve value for set. sk_tx_queue_get returns -1 on
invalid and the callers were validating -ve values. With
sk_tx_queue_mapping initialized to USHRT_MAX, and having an additional
check in sk_tx_queue_get to return -1 if sk_tx_queue_mapping has
USHRT_MAX, I think I can keep changes minimal and avoid the arithmetic
if that's a more acceptable solution.
quoted
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
quoted
}
static inline void sk_set_socket(struct sock *sk, struct socket *sock)