Re: [PATCH -next 0/3] tc state machinery cleanups

[Jamal Hadi Salim <jhs@mojatatu.com>]

Hi Florian,

On 05/15/15 04:50, Florian Westphal wrote:
> This series prepares removal of tc_verd member from sk_buff.
>
> It simplifies tc state machinery to what is required to keep current
> mirred/ifb combinations working.
>
> I tested a few scenarios, namely:
>
> 1 - htb based shaping on egress
> 2 - netem attached to ifb with mirred redirect from ingress qdisc
> 3 - mirred to different egress device
> 4 - mirred to ifb egress device with qdiscs set up on ifb
>      to provide illusion of 'single' transmit interface for traffic shaping
>
> After this series tc_verd is only used by ifb to skip actions on egress.
>
> Part #2 of this series will remove tc_verd completely.
>

The major goal here is to release some of the bits that are
no longer in use. I am for that but struggling with the unintended
consequence of changing readability or subtle meanings.
I brought this up last time when you removed the 1-bit macro.
Think of someone who read the code 6  months ago.
It would be useful to keep readability for the rest of the code the
way it was - change the definition of the macros.
This sounds doable because 8 contigous bits are going to be available.
Note: The macros are fugly - but there are better tools these days to
make them easier to read so de-uglifying could be part of the goal.
Therefore, I suggest the initial effort should be to find ways to
co-locate the bits and save the 8  bits and unless necessary keep
the code readability.

More on readability and subtle meanings:
A noun like "skb_tc_state" is not a good choice. Those two bits
carry location indicators and are intended to define source
and destination endpoints.

Also note: ifb and mirred are using the location metadata
bits - they are _not_ the owners of those bits or the only
potential users. Any of the blocks can consume or produce the metadatum
in order to aid policy traversal. You can use those two as examples
of existing blocks that do so - but annotating the code as such is
inaccurate.

cheers,
jamal