Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751573AbdHONwK (ORCPT ); Tue, 15 Aug 2017 09:52:10 -0400 Received: from mail-qt0-f172.google.com ([209.85.216.172]:32925 "EHLO mail-qt0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751070AbdHONwI (ORCPT ); Tue, 15 Aug 2017 09:52:08 -0400 MIME-Version: 1.0 In-Reply-To: <20170815130806.25168-1-mohamed.a.alrshah@ieee.org> References: <20170815130806.25168-1-mohamed.a.alrshah@ieee.org> From: Neal Cardwell Date: Tue, 15 Aug 2017 09:51:36 -0400 Message-ID: Subject: Re: [PATCH] Adding-Agile-SD-TCP-module-and-modifying-Kconfig-and-makefile To: mohamedalrshah Cc: David Miller , Netdev , Linus Torvalds , LKML , Mohamed Othman , Borhanuddin Ali , Zurina Hanapi Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5517 Lines: 148 On Tue, Aug 15, 2017 at 9:08 AM, mohamedalrshah wrote: > + > +/* Agile-SD Parameters */ > +struct agilesdtcp { > + u32 loss_cwnd; /* congestion window at last loss.*/ Please rebase your change on top of the latest net-next changes and update this module to use the latest approach from the recent commit: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=f1722a1be19dc38e0a4b282d4e6e6ec5e1b11a67 tcp: consolidate congestion control undo functions Specifically: - reference tp->prior_cwnd instead of ca->loss_cwnd - remove the ca->loss_cwnd field - have the .undo_cwnd field reference tcp_reno_undo_cwnd > + u32 frac_tracer; /* This is to trace the fractions of the increment.*/ > + u32 degraded_loss_cwnd; /* loss_cwnd after degradation.*/ > + enum dystate{SS=0, CA=1} agilesd_tcp_status; Per Linux style, please define the enum separately before declaring the variable of that type, and format the enum using Linux style. Also please use a longer, more specific name, to avoid name collisions. I'd suggest: enum dystate { AGILE_SD_SS = 0, /* comment ... */ AGILE_SD_CA = 1, /* comment ... */ }; > +}; > + > +/* To reset the parameters if needed*/ > +static inline void agilesdtcp_reset(struct sock *sk) > +{ > + > +} > + > +/* This function is called after the first acknowledgment is received and before the congestion > + * control algorithm will be called for the first time. If the congestion control algorithm has > + * private data, it should initialize its private date here. */ Multi-line comments should end with the trailing */ on a line by itself. Here and elsewhere. Please read: https://www.kernel.org/doc/html/v4.10/process/coding-style.html Please check the style of patches before submitting with the following script in the Linux kernel tree: scripts/checkpatch.pl > +static void agilesdtcp_init(struct sock *sk) > +{ > + struct agilesdtcp *ca = inet_csk_ca(sk); > + > + /* The value of initial_ssthresh parameter is not used here, thus, snd_ssthresh is initialized by a large value.*/ > + tcp_sk(sk)->snd_ssthresh = 0x7fffffff; > + > + ca->loss_cwnd = 0; > + ca->frac_tracer = 0; > + ca->agilesd_tcp_status = SS; > +} > + > +/* This function is called whenever an ack is received and the congestion window can be increased. > + * This is equivalent to opencwnd in tcp.cc. > + * ack is the number of bytes that are acknowledged in the latest acknowledgment; > + * rtt is the the rtt measured by the latest acknowledgment; > + * in_flight is the packet in flight before the latest acknowledgment; > + * good_ack is an indicator whether the current situation is normal (no duplicate ack, no loss and no SACK). */ > +static void agilesdtcp_cong_avoid(struct sock *sk, u32 ack, u32 in_flight) > +{ > + struct tcp_sock *tp = tcp_sk(sk); > + struct agilesdtcp *ca = inet_csk_ca(sk); > + u32 inc_factor; > + u32 ca_inc; > + u32 current_gap, total_gap; For coding style, please order local variable declarations from longest to shortest line, also know as Reverse Christmas Tree Format. > + /* The value of inc_factor is limited by lower_fl and upper_fl. > + * The lower_fl must be always = 1. The greater the upper_fl the higher the aggressiveness. > + * But, if upper_fl set to 1, Agile-SD will work exactly as newreno. > + * We have already designed an equation to calculate the optimum upper_fl based on the given beta. > + * This equation will be revealed once its article is published*/ > + u32 lower_fl = 1 * SCALE; > + u32 upper_fl = 3 * SCALE; > + > + if (!tcp_is_cwnd_limited(sk)) return; Please put this return (or any if/else body) on a line by itself. > + > + if (tp->snd_cwnd < tp->snd_ssthresh){ Need a space between ) and {. > + ca->agilesd_tcp_status = SS; > + tcp_slow_start(tp, in_flight); > + } > + else { The else needs to go on the same line as the closing brace. > + inc_factor = min(max(((upper_fl * current_gap) / total_gap), lower_fl), upper_fl); Please use the existing kernel helper macro for this: #define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi) > + > + ca_inc = ((inc_factor * SCALE) / tp->snd_cwnd); /* SCALE is used to avoid fractions*/ > + > + ca->frac_tracer += ca_inc; /* This in order to take the fraction increase into account */ > + if (ca->frac_tracer >= Double_SCALE) /* To take factor scale into account */ > + { The opening brace goes on the previous line. > +/* This function is called when the TCP flow detects a loss. > + * It returns the slow start threshold of a flow, after a packet loss is detected. */ Trailing */ style issue again. > +static u32 agilesdtcp_recalc_ssthresh(struct sock *sk) > +{ > + const struct tcp_sock *tp = tcp_sk(sk); > + struct agilesdtcp *ca = inet_csk_ca(sk); > + > + ca->loss_cwnd = tp->snd_cwnd; > + > + if (ca->agilesd_tcp_status == CA) > + ca->degraded_loss_cwnd = max((tp->snd_cwnd * beta) / SCALE, 2U); > + else > + ca->degraded_loss_cwnd = max((tp->snd_cwnd * beta) / SCALE, 2U); These two branches of the if/else look the same? Can they be condensed to a single line? Thanks, neal