Re: Question about HFSC atm+man patches

Re: Question about HFSC atm+man patches

[Patrick McHardy]

Michal Soltys wrote:
> I'm thinking about writing few simple patches that would introduce layer
> 2 adaptation to HFSC. Generally - instead of preparing adjusted rate
> table like some of the other qdiscs do (tbf, htb, ...), hfsc could
> simply adjust packets' lengths in qdisc code (and there are just few 
> places where it's done) - as it's used for all time / curve related 
> computations later. So the whole thing should be, at least in theory, 
> pretty straightforward.
> 
> The tc's interface would of course remain the same - that is - linklayer
> and overhead options, consistently with other qdiscs.
> 
> Would such approach be valid and possibly accepted ?

We already support generic size adjustment for all qdiscs. I'm
not sure about the userspace interface though.

> Another thing I'd like to add is detailed tc-hfsc man page (and maybe
> add/update other ones - but that's for a bit later).

That would be great.

Re: Question about HFSC atm+man patches

[Michal Soltys]

Patrick McHardy wrote:
> Michal Soltys wrote:
>> I'm thinking about writing few simple patches that would introduce layer
>> 2 adaptation to HFSC. Generally - instead of preparing adjusted rate
>> table like some of the other qdiscs do (tbf, htb, ...), hfsc could
>> simply adjust packets' lengths in qdisc code (and there are just few 
>> places where it's done) - as it's used for all time / curve related 
>> computations later. So the whole thing should be, at least in theory, 
>> pretty straightforward.
>>
>> The tc's interface would of course remain the same - that is - linklayer
>> and overhead options, consistently with other qdiscs.
>>
>> Would such approach be valid and possibly accepted ?
> 
> We already support generic size adjustment for all qdiscs. I'm
> not sure about the userspace interface though.
> 

More about what I have in mind - in april '08 there was set of 8 patches
-
http://kerneltrap.org/mailarchive/linux-netdev/2008/4/9/1386524/thread#mid-1386524

They added atm adaptation to tbf, htb, cbq and filter's action police.
The rate table is precalculated in userspace, and depending on tc
invocation - will be either unaltered, or the times will be set
according to lengths aligned to atm cells (tc_calc_rtable, in tc/tc_core.c).

On the kernel's side, rata table is consulted through qdisc_l2t
function, where overhead and cell_align are taken into account as well.

Anyway - HFSC wasn't included in that patchset, as it didn't use rate
table - so what I have in mind is essentially:

len = hfsc_l2_adapt(q->overhead,qdisc_pkt_len(skb));

instead of

len = qdisc_pkt_len(skb);

+ setup from tc and hfsc_init (overhead parameter, pointing
hfsc_l2_adapt to required function, sanity checks, etc.).

>> Another thing I'd like to add is detailed tc-hfsc man page (and maybe
>> add/update other ones - but that's for a bit later).
> 
> That would be great.

Allright, will do.


ps.

Removed linux-net from CC.

Re: Question about HFSC atm+man patches

[Patrick McHardy]

Michal Soltys wrote:
> Patrick McHardy wrote:
>> Michal Soltys wrote:
>>> Would such approach be valid and possibly accepted ?
>>
>> We already support generic size adjustment for all qdiscs. I'm
>> not sure about the userspace interface though.
>>
> 
> More about what I have in mind - in april '08 there was set of 8 patches
> -
> http://kerneltrap.org/mailarchive/linux-netdev/2008/4/9/1386524/thread#mid-1386524 
> 
> 
> They added atm adaptation to tbf, htb, cbq and filter's action police.
> The rate table is precalculated in userspace, and depending on tc
> invocation - will be either unaltered, or the times will be set
> according to lengths aligned to atm cells (tc_calc_rtable, in 
> tc/tc_core.c).
> 
> On the kernel's side, rata table is consulted through qdisc_l2t
> function, where overhead and cell_align are taken into account as well.
> 
> Anyway - HFSC wasn't included in that patchset, as it didn't use rate
> table - so what I have in mind is essentially:
> 
> len = hfsc_l2_adapt(q->overhead,qdisc_pkt_len(skb));
> 
> instead of
> 
> len = qdisc_pkt_len(skb);

qdisc_pkt_len already returns an adjusted length based on "size tables".
Check out qdisc_enqueue()/qdisc_enqueue_root() for details.

Is there something missing for overhead calculation?

Re: Question about HFSC atm+man patches

[Michal Soltys]

Patrick McHardy wrote:
> 
> qdisc_pkt_len already returns an adjusted length based on "size tables".
> Check out qdisc_enqueue()/qdisc_enqueue_root() for details.
> 
> Is there something missing for overhead calculation?

Thanks for pointing that out. It will fit well.

I'll have to wrap my head around the sources first though (even if the 
concept became even simpler now) - the patches for hfsc submitted in 
July along the ones for size tables, were a bit more extensive compared 
to what I've planned.

Re: Question about HFSC atm+man patches

[Michal Soltys]

One question regarding size tables:

Should I assume that subsequent qdiscs attached to HFSC leaves will not 
use [different] size tables ?

This is currently the case, but from the perspective of a generic 
solution - if some inner qdisc changes pkt_len stored in cb area during 
qdisc_enqueue, then HFSC might get wrong size during peek operation.

As leaves will usually have "simple" work-conserving qdiscs attached, 
this is probably not an issue, but ...

Re: Question about HFSC atm+man patches

[Patrick McHardy]

Michal Soltys wrote:
> One question regarding size tables:
> 
> Should I assume that subsequent qdiscs attached to HFSC leaves will not 
> use [different] size tables ?
> 
> This is currently the case, but from the perspective of a generic 
> solution - if some inner qdisc changes pkt_len stored in cb area during 
> qdisc_enqueue, then HFSC might get wrong size during peek operation.
> 
> As leaves will usually have "simple" work-conserving qdiscs attached, 
> this is probably not an issue, but ...

Using different size tables for inner qdiscs is supposed to work.
What is the "wrong size"? ->peek() should return a packet with the
size the same as ->dequeue() would, which I think it does.

Re: Question about HFSC atm+man patches

[Michal Soltys]

Patrick McHardy wrote:
> 
> Using different size tables for inner qdiscs is supposed to work. 
> What is the "wrong size"? ->peek() should return a packet with the 
> size the same as ->dequeue() would, which I think it does.
> 

That's not what I meant.

((struct qdisc_skb_cb*)skb->cb)->pkt_len

This is the value returned by qdisc_pkt_len() (now used whenever length 
is required, instead of skb->len), and it's recalculated during 
qdisc_calculate_pkt_len() called from qdisc_enqueue() - if a qdisc has 
stab. So the inner(most) qdisc with a size table, will be the last one 
setting that field.

Now, if an outer qdisc, e.g. HFSC, does peek() or dequeue() later, 
qdisc_pkt_len() will return overridden value.

If no qdiscs in "chain" have stab, then the field will be equal to 
skb->len - which is set in qdisc_enqueue_root() (otherwise uninitialzed 
value would be used later).

Am I seeing this right ?

Question - size tables implementation

[Michal Soltys]

Hi

This is a followup question, continuing from:

http://marc.info/?l=linux-netdev&m=122782976032316&w=2

Well, at least this is what I'm seeing from looking at the code. Perhaps 
I missed something.

Assuming I didn't - an example of a flow in a simple example (let's say 
- hypothetical "outer" as a root qdisc, and "inner" as a leaf qdisc - 
both using different size tables):

1) qdisc_enqueue_root() will set pkt_len in cb area to skb->len, call 
qdisc_enqueue(), then

2) qdisc_calculate_pkt_len() will set pkt_len in cb area using "outer"'s 
size table, and

3) outer_enqueue() will be will be called. This one will classify the 
packet and

4) qdisc_enqueue() it to the "inner" qdisc, but that function will 
qdisc_calculate_pkt_len() overwriting pkt_len in cb area using "inner"'s 
size table

Later, whenever the time comes to e.g. dequeue or peek at the packet, 
"outer" will see packet size set in (4) whenever it calls qdisc_pkt_len().

For now, as far as I can see, the stab functionality is ready to be 
used, but no scheduler actually uses it - so the whole chain ends at 
setting ((struct qdisc_skb_cb*)skb->cb)->pkt_len during 
qdisc_enqueue_root() to skb->len.

If I was to add atm adaptation to HFSC using size tables (as suggested 
by Patrick), I'd like to know which direction to choose:

1) be prepared for it (e.g. save pkt_len, rerun 
qdisc_calculate_pkt_len() every time adjusted length is needed, restore 
the previous pkt_len afterwards) - but it's ugly and bloaty, and causes 
plenty of unnecessary calls to the calculate function.

2) change size table implementation, e.g.

- add depth field in struct Qdisc {} - 0 for root, and parent's
sch->depth+1 for any deeper qdisc, during qdisc creation
- make qdisc_calculate_pkt_len() and qdisc_pkt_len() use that as an
index, so instead of

struct qdisc_skb_cb {
         unsigned int            pkt_len;
         char                    data[];
};

we would have something like:

struct qdisc_skb_cb {
         unsigned int            pkt_len[QDISC_MAX_DEPTH];
         char                    data[];
};

static inline unsigned int qdisc_pkt_len(struct sk_buff *skb, struct
Qdisc *sch)
{
         return qdisc_skb_cb(skb)->pkt_len[sch->depth];
}

etc.

3) Ignore ? Aka assume only outer (or non-work conserving) qdisc may use 
size tables, but that's just wrong...