On 29-May-18 2:05 AM, Jakub Kicinski wrote:
Hi Jakub,
Hi Nogah!
On Mon, 28 May 2018 18:49:51 +0300, Nogah Frankel wrote:
+static int
+nfp_abm_red_replace(struct net_device *netdev, struct nfp_abm_link *alink,
+ struct tc_red_qopt_offload *opt)
+{
+ struct nfp_port *port = nfp_port_from_netdev(netdev);
+ int err;
+
+ if (opt->set.min != opt->set.max || !opt->set.is_ecn) {
I am a bit worried about the min == max.
sch_red doesn't really support it. It will calculate incorrect delta
value. (And that only if tc_red_eval_P in iproute2 won't reject it).
You might maybe use max = min+1, because in real life it will probably
act the same but without this problem.
I remember having a long think about this when I wrote the code.
My conclusion was that the two would operate almost the same, and
setting min == max may be most obvious to the user.
I agree.
If min + 1 == max sch_red would act probabilistically for qavg == min,
which is not what the card would do.
Userspace now does this:
tc_red_eval_P() {
int i = qmax - qmin;
if (!i)
return 0;
if (i < 0)
return -1;
...
}
And you've fixed delta to be treated as 1 to avoid division by 0 in
commit 5c472203421a ("net_sched: red: Avoid devision by zero"):
red_set_parms() {
int delta = qth_max - qth_min;
u32 max_p_delta;
p->qth_min = qth_min << Wlog;
p->qth_max = qth_max << Wlog;
p->Wlog = Wlog;
p->Plog = Plog;
if (delta <= 0)
delta = 1;
p->qth_delta = delta;
...
}
I changes it to avoid division by 0, but I wasn't sure that the delta
value of 1 will be good, just that it is better then 0.
So we should be safe. Targets will match. Probability adjustment for
adaptive should work correctly. Which doesn't matter anyway, since we
will never use the probabilistic action...
That makes sense, and it is a better way to set this setup (DCTCP, I
guess?) than before.
Thanks
Nogah
Nogah Frankel
(from a new mail address)
Noted :)
