Re: [PATCH net-next 3/7] net: hns3: add support for forwarding packet to queues of specified TC when flow director rule hit

[tanhuazhong]

On 2020/12/10 13:40, Saeed Mahameed wrote:
> On Thu, 2020-12-10 at 11:42 +0800, Huazhong Tan wrote:
> > From: Jian Shen <shenjian15@xxxxxxxxxx>
> >
> > For some new device, it supports forwarding packet to queues
> > of specified TC when flow director rule hit. So extend the
> > command handle to support it.
>
> ...
>
> >   static int hclge_config_action(struct hclge_dev *hdev, u8 stage,
> >   			       struct hclge_fd_rule *rule)
> >   {
> > +	struct hclge_vport *vport = hdev->vport;
> > +	struct hnae3_knic_private_info *kinfo = &vport->nic.kinfo;
> >   	struct hclge_fd_ad_data ad_data;
> >   
> > +	memset(&ad_data, 0, sizeof(struct hclge_fd_ad_data));
> >   	ad_data.ad_id = rule->location;
> >   
> >   	if (rule->action == HCLGE_FD_ACTION_DROP_PACKET) {
> >   		ad_data.drop_packet = true;
> > -		ad_data.forward_to_direct_queue = false;
> > -		ad_data.queue_id = 0;
> > +	} else if (rule->action == HCLGE_FD_ACTION_SELECT_TC) {
> > +		ad_data.override_tc = true;
> > +		ad_data.queue_id =
> > +			kinfo->tc_info.tqp_offset[rule->tc];
> > +		ad_data.tc_size =
> > +			ilog2(kinfo->tc_info.tqp_count[rule->tc]);
>
> In the previous patch you copied this info from mqprio, which is an
> egress qdisc feature, this patch is clearly about rx flow director, I
> think the patch is missing some context otherwise it doesn't make any
> sense.

Since tx and rx are in the same tqp, what we do here is to make tx and 
rx in the same tc when rule is hit.

> >   	} else {
> > -		ad_data.drop_packet = false;
> >   		ad_data.forward_to_direct_queue = true;
> >   		ad_data.queue_id = rule->queue_id;
> >   	}
> > @@ -5937,7 +5950,7 @@ static int hclge_add_fd_entry(struct
> > hnae3_handle *handle,
> >   			return -EINVAL;
> >   		}
> >   
> > -		action = HCLGE_FD_ACTION_ACCEPT_PACKET;
> > +		action = HCLGE_FD_ACTION_SELECT_QUEUE;
> >   		q_index = ring;
> >   	}
> >   
> > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > index b3c1301..a481064 100644
> > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> > @@ -572,8 +572,9 @@ enum HCLGE_FD_PACKET_TYPE {
> >   };
> >   
> >   enum HCLGE_FD_ACTION {
> > -	HCLGE_FD_ACTION_ACCEPT_PACKET,
> > +	HCLGE_FD_ACTION_SELECT_QUEUE,
> >   	HCLGE_FD_ACTION_DROP_PACKET,
> > +	HCLGE_FD_ACTION_SELECT_TC,
>
> what is SELECT_TC ? you never actually write this value anywhere  in
> this patch.

HCLGE_FD_ACTION_SELECT_TC means that the packet will be forwarded into 
the queue of specified TC when rule is hit.

the assignment is in the next patch, maybe these two patch should be 
merged for making it more readable.


Thanks.
Huazhong.

> .