From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Nambiar, Amritha" Subject: Re: [net-next PATCH v4 3/7] net: sock: Change tx_queue_mapping in sock_common to unsigned short Date: Tue, 26 Jun 2018 16:54:19 -0700 Message-ID: References: <152994950582.9733.3330634251364177102.stgit@anamhost.jf.intel.com> <152994986976.9733.18263514750793164132.stgit@anamhost.jf.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Linux Kernel Network Developers , "David S. Miller" , Alexander Duyck , Willem de Bruijn , Sridhar Samudrala , Eric Dumazet , Hannes Frederic Sowa To: Alexander Duyck , Tom Herbert Return-path: Received: from mga14.intel.com ([192.55.52.115]:26253 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934223AbeFZXyV (ORCPT ); Tue, 26 Jun 2018 19:54:21 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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) >>> >>