RE: [net-next PATCH] net: act_skbedit: Fix tc action skbedit queue_mapping

["Nambiar, Amritha" <amritha.nambiar@intel.com>]

> -----Original Message-----
> From: Alexander Duyck <alexander.duyck@gmail.com>
> Sent: Monday, August 9, 2021 5:51 PM
> To: Nambiar, Amritha <amritha.nambiar@intel.com>
> Cc: Netdev <netdev@vger.kernel.org>; Jakub Kicinski <kuba@kernel.org>;
> Jamal Hadi Salim <jhs@mojatatu.com>; Jiri Pirko <jiri@resnulli.us>; Cong
> Wang <xiyou.wangcong@gmail.com>; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>
> Subject: Re: [net-next PATCH] net: act_skbedit: Fix tc action skbedit
> queue_mapping
> 
> On Mon, Aug 9, 2021 at 4:36 PM Amritha Nambiar
> <amritha.nambiar@intel.com> wrote:
> >
> > For skbedit action queue_mapping to select the transmit queue,
> > queue_mapping takes the value N for tx-N (where N is the actual
> > queue number). However, current behavior is the following:
> > 1. Queue selection is off by 1, tx queue N-1 is selected for
> >    action skbedit queue_mapping N. (If the general syntax for queue
> >    index is 1 based, i.e., action skbedit queue_mapping N would
> >    transmit to tx queue N-1, where N >=1, then the last queue cannot
> >    be used for transmit as this fails the upper bound check.)
> > 2. Transmit to first queue of TCs other than TC0 selects the
> >    next queue.
> > 3. It is not possible to transmit to the first queue (tx-0) as
> >    this fails the bounds check, in this case the fallback
> >    mechanism for hash calculation is used.
> >
> > Fix the call to skb_set_queue_mapping(), the code retrieving the
> > transmit queue uses skb_get_rx_queue() which subtracts the queue
> > index by 1. This makes it so that "action skbedit queue_mapping N"
> > will transmit to tx-N (including the first and last queue).
> >
> > Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > ---
> >  net/sched/act_skbedit.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> > index e5f3fb8b00e3..a7bba15c74c4 100644
> > --- a/net/sched/act_skbedit.c
> > +++ b/net/sched/act_skbedit.c
> > @@ -59,7 +59,7 @@ static int tcf_skbedit_act(struct sk_buff *skb, const
> struct tc_action *a,
> >         }
> >         if (params->flags & SKBEDIT_F_QUEUE_MAPPING &&
> >             skb->dev->real_num_tx_queues > params->queue_mapping)
> > -               skb_set_queue_mapping(skb, params->queue_mapping);
> > +               skb_set_queue_mapping(skb, params->queue_mapping + 1);
> >         if (params->flags & SKBEDIT_F_MARK) {
> >                 skb->mark &= ~params->mask;
> >                 skb->mark |= params->mark & params->mask;
> >
> 
> I don't think this is correct. It is conflating the rx_queue_mapping
> versus the Tx queue mapping. This is supposed to be setting the Tx
> queue mapping which applies after we have dropped the Rx queue
> mapping, not before. Specifically this is run at the qdisc enqueue
> stage with a single locked qdisc, after netdev_pick_tx and skb_tx_hash
> have already run. It is something that existed before mq and is meant
> to work with the mutliq qdisc.
> 
> If you are wanting to add a seperate override to add support for
> programming the Rx queue mapping you may want to submit that as a
> different patch rather than trying to change the existing Tx queue
> mapping feature. Either that or you would need to change this so that
> it has a different behavior depending on where the hook is added since
> the behavior would be different if this is called before skb_tx_hash.

Hi Alex,
Thanks for the review. The goal is to select a transmit queue using tc egress rule
and the action skbedit (that will go through netdev_pick_tx and skb_tx_hash).
I am not sure of the correct syntax for the queue-mapping value in the
action (tx-N or tx-N+1). As per the man page
(https://man7.org/linux/man-pages/man8/tc-skbedit.8.html), I interpreted
it as "action skbedit queue_mapping N" will transmit to tx-N. But, the
3 observations I listed don't quite seem to be following the tc rule.
Hence, tried to fix this in the action module. 

-Amritha