Hi, Thomas The input set of control packet filter are dst_mac and ethertype in Ethernet head. To be clear, I think it's better to use the name ethertype filter.
While there is already ethertype filter existing in igb and ixgbe driver. I will rename The patchset to ethertype filter and also integrate igb and ixgbe's ethertype filter To the filter_ctrl API. What do you think? Jingjing > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Friday, October 31, 2014 4:45 PM > To: Wu, Jingjing > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/3] ethdev: define ctrl_pkt filter type > and its structure > > Hi Jingjing, > > I'm sorry, but your explanations are not sufficient. > Please keep in mind that the user of the API don't know i40e internals. > > 2014-10-31 07:05, Wu, Jingjing: > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > > 2014-10-22 16:19, Jingjing Wu: > > > > +/** > > > > + * Define all structures for Control Packet Filter type > > > > +corresponding with > > > specific operations. > > > > + */ > > > > > > Please explain what is a control packet. > > > > A control element in Fortville can be used to receive control packets and > control other switching elements. Control packet filter can filter control > packet (such as LLDP) to different queues in receive and identify the switch > element that should process the packets in transmit. > > At the same time, we also can use this filter to route non-control packets > > to > queue or other engine by filtering mac and ether_type. The name "control > packet filter" comes from Fortville. > > I still don't know what is a control packet. > > > > > +#define RTE_CONTROL_PACKET_FLAGS_IGNORE_MAC 0x0001 > > > > +#define RTE_CONTROL_PACKET_FLAGS_DROP 0x0002 > > > > +#define RTE_CONTROL_PACKET_FLAGS_TO_QUEUE 0x0004 > > > > +#define RTE_CONTROL_PACKET_FLAGS_TX 0x0008 > > > > +#define RTE_CONTROL_PACKET_FLAGS_RX 0x0000 > > > > > > Flag RX is 0? > > > > Yes, RX is default value. Maybe it need to be removed. > > No, it doesn't need to be removed. But a flag must not be 0. > 0 means none. > It's impossible to disable this flag. > > Moreover, you should comment each flag. > > > > > +/** > > > > + * A structure used to define the control packet filter entry > > > > + * to support RTE_ETH_FILTER_CTRL_PKT with RTE_ETH_FILTER_ADD > > > > + * and RTE_ETH_FILTER_DELETE operations. > > > > + */ > > > > +struct rte_ctrl_pkt_filter { > > > > + struct ether_addr mac_addr; /**< mac address to match. */ > > > > + uint16_t ether_type; /**< ether type to match */ > > > > + uint16_t flags; /**< options for filter's > > > > behavior*/ > > > > + uint16_t dest_id; /**< destination vsi id or pool > > > > id*/ > > > > > > vsi id and pool id cannot be understood in a generic context. > > > Please explain what you mean and why queue is not enough. > > > > If queue is not specified, dest_id defines which element (vsi) will get the > packet. > > If queue is specified, the queue need belong to the destination element. > > You really need to define what is a vsi id and pool id. These notions are not > known in the API layer. > > > > > + uint16_t queue; /**< queue assign to if TO QUEUE > > > > flag is set > > > */ > > > > > > TO QUEUE is not defined. Is it really needed? > > > > > TO QUEUE is just the flag RTE_CONTROL_PACKET_FLAGS_TO_QUEUE > above. > > OK, please use the same wording or users will get lost. > > -- > Thomas
