clarify logic & improve readability of select tin

[ldir-EDB0]

Existing code structure is difficult to read, also it looks up tc priority fields for tin selection even if running in a 1 tin only mode (besteffort)

Refactor code to only do tin look ups in classification modes that require it.

Clarify to us & compiler that DSCP washing is a boolean do/don't wash flag and the masked flag value isn't important.

Signed-off-by: Kevin Darbyshire-Bryant ldir@darbyshire-bryant.me.uk

[tohojo]

While you're cleaning this up, why not move the wash check to the end of the function (before the return) instead of duplicating the check in each branch? I'd just remove it from cake_handle_diffserv() as well...

[ldir-EDB0]

I started going down that road and fell over things that didn't 'feel right'. cake_handle_diffserv only washes if a) washing enabled AND b) dscp !=0 (ie. something to clean). It also does that whilst it knows which flavour of IP it's dealing with.

Moving washing to the end of select tin means that a) we need to look up IP type twice and b) in the case of diffserv we do the washing whether the DSCP needs it or not.

To my mind doing the laundry is expensive and we should only do it if required. Thoughts?

[tohojo]

The number of comparisons will be the same (another check against the IP version instead of a check against the DSCP value in handle_diffserv).

You may have a point about not rewriting DSCP if it's not set. I'm not actually sure it will make any measurable difference; but if it does, that would apply also in the case where we are not using the DSCP field to select a tin, so the check should be in cake_wash_diffserv for all cases, then.

Which means we could just get rid of cake_wash_diffserv(). Instead, simply call cake_handle_diffserv at the top of cake_select_tin, store the return value, and use it to select the tin later if appropriate...

[ldir-EDB0]

Ha, so I did the 'only wash if something launder' commit before I saw your comment. I'll re-think again :-). And yes I pondered a variation of what you suggested as well :-)

[tohojo]

Kevin Darbyshire-Bryant notifications@github.com writes:

Ha, so I did the 'only wash if something launder' commit before I saw your comment. I'll re-think again :-). And yes I pondered a variation of what you suggested as well :-)

Cool; please squash everything into a single commit when you're done :)

-Toke

[moeller0]

Silly question,

will actively remapping dscp 0 to dscp zero avoid having to recalculate the TCP checksum again?

Best Regards

On Mar 1, 2019, at 14:04, Toke Høiland-Jørgensen notifications@github.com wrote:

Kevin Darbyshire-Bryant notifications@github.com writes:

Ha, so I did the 'only wash if something launder' commit before I saw your comment. I'll re-think again :-). And yes I pondered a variation of what you suggested as well :-)

Cool; please squash everything into a single commit when you're done :)

-Toke — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[tohojo]

moeller0 notifications@github.com writes:

Silly question,

will actively remapping dscp 0 to dscp zero avoid having to recalculate the TCP checksum again?

Erm, no, in theory not. But the code does it anyway (see ipv4_change_dsfield()).

-Toke

[tohojo]

So I went ahead and implemented this (in 29d707ee1076e39ab0a03046a54e4030a1b002f0); sorry, but want to submit upstream today, and you were taking too long :P

[ldir-EDB0]

@moeller0 Well ipv4_change_dsfield expands (inline) to:

static inline void ipv4_change_dsfield(struct iphdr *iph,u8 mask, u8 value) { u32 check = ntohs((force be16)iph->check); u8 dsfield;

    dsfield = (iph->tos & mask) | value;
    check += iph->tos;
    if ((check+1) >> 16) check = (check+1) & 0xffff;
    check -= dsfield;
    check += check >> 16; /* adjust carry */
    iph->check = (__force __sum16)htons(check);
    iph->tos = dsfield;

}