Re: [dpdk-dev] [PATCH v2 1/3] ethdev: define ctrl_pkt filter type and its structure

["Wu, Jingjing" <jingjing.wu@intel.com>]

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@6wind.com]
> Sent: Friday, October 31, 2014 4:45 PM
> To: Wu, Jingjing
> Cc: dev@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@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