From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp2120.oracle.com ([141.146.126.78]:51178 "EHLO aserp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751416AbeCUQRA (ORCPT ); Wed, 21 Mar 2018 12:17:00 -0400 Subject: Re: [PATCH net-next 01/14] tcp: Add clean acked data hook To: Boris Pismenny , Saeed Mahameed , "David S. Miller" Cc: netdev@vger.kernel.org, Dave Watson , Ilya Lesokhin , Aviad Yehezkel References: <20180320024510.7408-1-saeedm@mellanox.com> <20180320024510.7408-2-saeedm@mellanox.com> <1dc8963f-a279-b03c-c0b8-805f10e1c41a@oracle.com> <437e3972-e88d-bee3-b6bb-9853867002f0@mellanox.com> From: Rao Shoaib Message-ID: <34fcec92-dfc8-644d-ed54-c54eab71ffb6@oracle.com> Date: Wed, 21 Mar 2018 09:16:48 -0700 MIME-Version: 1.0 In-Reply-To: <437e3972-e88d-bee3-b6bb-9853867002f0@mellanox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 03/21/2018 04:21 AM, Boris Pismenny wrote: > > > On 3/20/2018 10:36 PM, Rao Shoaib wrote: >> >> >> On 03/19/2018 07:44 PM, Saeed Mahameed wrote: >>> From: Ilya Lesokhin >>> >>> Called when a TCP segment is acknowledged. >>> Could be used by application protocols who hold additional >>> metadata associated with the stream data. >>> >>> This is required by TLS device offload to release >>> metadata associated with acknowledged TLS records. >>> >>> Signed-off-by: Ilya Lesokhin >>> Signed-off-by: Boris Pismenny >>> Signed-off-by: Aviad Yehezkel >>> Signed-off-by: Saeed Mahameed >>> --- >>>   include/net/inet_connection_sock.h | 2 ++ >>>   net/ipv4/tcp_input.c               | 2 ++ >>>   2 files changed, 4 insertions(+) >>> >>> diff --git a/include/net/inet_connection_sock.h >>> b/include/net/inet_connection_sock.h >>> index b68fea022a82..2ab6667275df 100644 >>> --- a/include/net/inet_connection_sock.h >>> +++ b/include/net/inet_connection_sock.h >>> @@ -77,6 +77,7 @@ struct inet_connection_sock_af_ops { >>>    * @icsk_af_ops           Operations which are AF_INET{4,6} specific >>>    * @icsk_ulp_ops       Pluggable ULP control hook >>>    * @icsk_ulp_data       ULP private data >>> + * @icsk_clean_acked       Clean acked data hook >>>    * @icsk_listen_portaddr_node    hash to the portaddr listener >>> hashtable >>>    * @icsk_ca_state:       Congestion control state >>>    * @icsk_retransmits:       Number of unrecovered [RTO] timeouts >>> @@ -102,6 +103,7 @@ struct inet_connection_sock { >>>       const struct inet_connection_sock_af_ops *icsk_af_ops; >>>       const struct tcp_ulp_ops  *icsk_ulp_ops; >>>       void              *icsk_ulp_data; >>> +    void (*icsk_clean_acked)(struct sock *sk, u32 acked_seq); >>>       struct hlist_node         icsk_listen_portaddr_node; >>>       unsigned int          (*icsk_sync_mss)(struct sock *sk, u32 >>> pmtu); >>>       __u8              icsk_ca_state:6, >>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >>> index 451ef3012636..9854ecae7245 100644 >>> --- a/net/ipv4/tcp_input.c >>> +++ b/net/ipv4/tcp_input.c >>> @@ -3542,6 +3542,8 @@ static int tcp_ack(struct sock *sk, const >>> struct sk_buff *skb, int flag) >>>       if (after(ack, prior_snd_una)) { >>>           flag |= FLAG_SND_UNA_ADVANCED; >>>           icsk->icsk_retransmits = 0; >>> +        if (icsk->icsk_clean_acked) >>> +            icsk->icsk_clean_acked(sk, ack); >>>       } >>>       prior_fack = tcp_is_sack(tp) ? tcp_highest_sack_seq(tp) : >>> tp->snd_una; >> Per Dave we are not allowed to use function pointers any more, so why >> extend their use. I implemented a similar callback for my changes but >> in my use case I need to call the meta data update function even when >> the packet does not ack any new data or has no payload. Is it >> possible to move this to say tcp_data_queue() ? > > Sometimes function pointers are unavoidable. For example, when a > module must change the functionality of a function. I think it is > preferable to advance the kernel I agree, in fact I was using function pointers for the exact reason, to change the functionality of a function. I asked Dave about the use and he said No (Also note that the relevant CPU optimizations have been turned off on selected NIC's due to the latest security issues -- On AMD CPU's the optimizations are not turned off). So it is Dave's decision -- I am hoping that he would reconsider and allow me to use pointers as well as pointers solve the problem nicely and are used extensively. > > This function is used to free memory based on new acknowledged data. > It is unrelated to whether data was received or not. So it is not > possible to move this call to tcp_data_queue. After reviewing my changes I believe I can work with the change. So go ahead. > > Just in case, I'll add a static key here to reduce the impact on the > fast-path as once suggested by EricD on netdev2.2. Regards, Shoaib > >> >> Thanks, >> >> Shoaib >> >> > > Best, > Boris.