On Wed, Oct 16, 2019 at 4:22 PM Jakub Kicinski <jakub.kicin...@netronome.com> wrote: > > On Wed, 16 Oct 2019 15:42:28 -0700, Cong Wang wrote: > > > @@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct > > > Qdisc *sch, > > > } > > > segs = skb2; > > > } > > > - qdisc_tree_reduce_backlog(sch, -nb, prev_len - len); > > > + qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len); > > > > Am I the only one has trouble to understand the expression > > "!skb - nb"? > > The backward logic of qdisc_tree_reduce_backlog() always gives me a > pause :S
Yeah, reducing with a negative value is actually an add. Feel free to add a wrapper for this if you think it is better. > > Is > -nb + !skb > any better? I don't see how they are different. :-/ > > The point is we have a "credit" for the "head" skb we dropped. If we > didn't manage to queue any of the segs then the expression becomes > ...reduce_backlog(sch, 1, prev_len) basically cleaning up after the > head. > > My knee jerk reaction was -> we should return DROP if head got dropped, > but that just makes things more nasty because we requeue the segs > directly into netem so if we say DROP we have to special case all the > segs which succeeded, that gets even more hairy. Hmm? My understanding is that !skb is either 0 or 1, so you end up with either "-nb" or "1 - nb". The formal is easy to understand, while the later is harder as I don't see why you need to plus 1. > > I'm open to suggestions.. :( Why not write the code in a more readable way, for instance with the :? operator? And, adding a comment in the code? Thanks.
