Re: [net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short
On 6/26/2018 3:58 AM, Willem de Bruijn wrote:
> On Mon, Jun 25, 2018 at 7:06 PM Amritha Nambiar
> wrote:
>>
>> 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
>> ---
>
>> 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);
>> + sk->sk_tx_queue_mapping = tx_queue + 1;
>> }
>
> WARN_ON_ONCE to avoid flooding the kernel buffer.
>
Will fix.
Re: [net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short
On 6/25/2018 8:25 PM, Alexander Duyck wrote:
> On Mon, Jun 25, 2018 at 6:34 PM, Tom Herbert wrote:
>>
>>
>> On Mon, Jun 25, 2018 at 11:04 AM, Amritha Nambiar
>> wrote:
>>>
>>> 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
>>> ---
>>> 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.
>
>>
>>> + 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.
>>>
>>> }
>>>
>>> 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.
>
>>
>>>
>>>
>>>
>>> }
>>>
>>> static inline void sk_set_socket(struct sock *sk, struct socket *sock)
>>>
>>
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 7:06 PM Amritha Nambiar
wrote:
>
> 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
> ---
> 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);
> + sk->sk_tx_queue_mapping = tx_queue + 1;
> }
WARN_ON_ONCE to avoid flooding the kernel buffer.
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 wrote:
>
>
> On Mon, Jun 25, 2018 at 11:04 AM, Amritha Nambiar
> wrote:
>>
>> 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
>> ---
>> 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.
>
>> + 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.
>>
>> }
>>
>> 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.
>
>>
>>
>>
>> }
>>
>> static inline void sk_set_socket(struct sock *sk, struct socket *sock)
>>
>
[net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short
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
---
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);
+ 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;
}
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;
}
static inline void sk_set_socket(struct sock *sk, struct socket *sock)
