On Wed, 2018-06-20 at 12:47 -0400, Michel Machado wrote:
> On 06/20/2018 12:08 PM, Davide Caratti wrote:
> > On Tue, 2018-06-12 at 15:42 +0000, Fu, Qiaobin wrote:
> > > The new action inheritdsfield copies the field DS of
> > > IPv4 and IPv6 packets into skb->priority. This enables
> > > later classification of packets based on the DS field.
> > >
> > > v4:
> > > *Not allow setting flags other than the expected ones.
> > >
> > > *Allow dumping the pure flags.
> > >
> > > Original idea by Jamal Hadi Salim <j...@mojatatu.com>
> > >
> > > Signed-off-by: Qiaobin Fu <qiaob...@bu.edu>
> > > Reviewed-by: Michel Machado <mic...@digirati.com.br>
> > > ---
> > >
> >
> > [...]
> >
> > > @@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const
> > > struct tc_action *a,
> > >
> > > if (d->flags & SKBEDIT_F_PRIORITY)
> > > skb->priority = d->priority;
> > > + if (d->flags & SKBEDIT_F_INHERITDSFIELD) {
> > > + int wlen = skb_network_offset(skb);
> > > +
> > > + switch (tc_skb_protocol(skb)) {
> > > + case htons(ETH_P_IP):
> > > + wlen += sizeof(struct iphdr);
> > > + if (!pskb_may_pull(skb, wlen))
> > > + goto err;
> > > + skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
> > > + break;
> > > +
> > > + case htons(ETH_P_IPV6):
> > > + wlen += sizeof(struct ipv6hdr);
> > > + if (!pskb_may_pull(skb, wlen))
> > > + goto err;
> > > + skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
> > > + break;
> > > + }
> > > + }
> > > if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
> > > skb->dev->real_num_tx_queues > d->queue_mapping)
> > > skb_set_queue_mapping(skb, d->queue_mapping);
> > > @@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const
> > > struct tc_action *a,
> > >
> > > spin_unlock(&d->tcf_lock);
> > > return d->tcf_action;
> > > +
> > > +err:
> > > + spin_unlock(&d->tcf_lock);
> > > + return TC_ACT_SHOT;
> > > }
> > >
> >
> > sorry for asking this when the patch is a v4...
> >
> > I spotted this, as I'm rebasing a small series that removes the tcf_lock
> > from the data plane of skbedit to gain some speed, and it converts the
> > stats to be per-cpu counters.
> >
> > in the code above, you are catching failures of pskb_may_pull(skb) and
> > then you return TC_ACT_SHOT. That's OK, but I think you should update the
> > drop counter, like other TC actions do.
> >
> > If you (author / reviewers) think this is a minor issue, like I do think,
> > then I can add the missing update in my series and post it when net-next
> > reopens.
> >
> > WDYT?
> >
> > thank you in advance!
> > regards,
> >
>
> Hi Davide,
>
> I agree that we should update the drop counter, but given that
> you're already converting the stats to be per-cpu counters, whatever we
> add now will be just symbolic since you're going to change it anyway.
that's ok for me also, as I can use the current v4 code for the rebase
(and not wait for another respin) _ but let's hear what reviewers think.
> If
> reviewers think that Qiaobin's patch must add the update line, could you
> provide the exact line and location so we avoid going to v6 of this patch?
In case, I was thinking of something like:
https://elixir.bootlin.com/linux/v4.18-rc1/source/net/sched/act_ipt.c#L249
so, between 'err:' and 'spin_unlock(&d->tcf_lock)', insert a line like:
d->tcf_qstats.drop++;
regards,
--
davide
