Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface

[Dimitris Michailidis <dm@chelsio.com>]

On 05/03/2011 09:12 AM, Alexander Duyck wrote:

[...]

> +static int rmgr_add(struct ethtool_rx_flow_spec *fsp)
> +{
> +	__u32 loc = fsp->location;

Unneeded assignment.  Couple lines later you assign a new value to loc.

> +	__u32 slot_num;
> +
> +	/* start at the end of the list since it is lowest priority */
> +	loc = rmgr.size - 1;
> +
> +	/* locate the first slot a rule can be placed in */
> +	slot_num = loc / BITS_PER_LONG;
> +
> +	/*
> +	 * Avoid testing individual bits by inverting the word and checking
> +	 * to see if any bits are left set, if so there are empty spots.  By
> +	 * moving 1 + loc % BITS_PER_LONG we align ourselves to the last bit
> +	 * in the previous word.
> +	 *
> +	 * If loc rolls over it should be greater than or equal to rmgr.size
> +	 * and as such we know we have reached the end of the list.
> +	 */
> +	if (!~(rmgr.slot[slot_num] | (~1UL << rmgr.size % BITS_PER_LONG))) {
> +		loc -= 1 + (loc % BITS_PER_LONG);
> +		slot_num--;
> +	}
> +
> +	/*
> +	 * Now that we are aligned with the last bit in each long we can just
> +	 * go though and eliminate all the longs with no free bits
> +	 */
> +	while (loc < rmgr.size && !~(rmgr.slot[slot_num])) {
> +		loc -= BITS_PER_LONG;
> +		slot_num--;
> +	}
> +
> +	/*
> +	 * If we still are inside the range, test individual bits as one is
> +	 * likely available for our use.
> +	 */
> +	while (loc < rmgr.size && test_bit(loc, rmgr.slot))
> +		loc--;
> +
> +	/* location found, insert rule */
> +	if (loc < rmgr.size) {
> +		fsp->location = loc;
> +		return rmgr_ins(loc);
> +	}
> +
> +	/* No space to add this rule */
> +	fprintf(stderr, "rmgr: Cannot find appropriate slot to insert rule\n");
> +
> +	return -1;
> +}

[...]

> +int rxclass_rule_ins(int fd, struct ifreq *ifr,
> +		     struct ethtool_rx_flow_spec *fsp)
> +{
> +	struct ethtool_rxnfc nfccmd;
> +	__u32 loc = fsp->location;
> +	int err;
> +
> +	/*
> +	 * if location is unspecified pull rules from device
> +	 * and allocate a free rule for our use
> +	 */
> +	if (loc == RX_CLS_LOC_UNSPEC) {
> +		/* init table of available rules */
> +		err = rmgr_init(fd, ifr);
> +		if (err < 0)
> +			return err;
> +
> +		/* verify rule location */
> +		err = rmgr_add(fsp);
> +		if (err < 0)
> +			return err;
> +
> +		/* cleanup table and free resources */
> +		rmgr_cleanup();
> +	}

This logic where ethtool tries to select a filter slot when a user provides 
RX_CLS_LOC_UNSPEC does not work in general.  It assumes that all slots are 
equal and a new filter can go into any available slot. But a device may have 
restrictions on where a filter may go that ethtool doesn't know.  I 
mentioned during a previous review that for cxgb4 some filters require a 
slot number that is a multiple of 4.  There are some other constraints as 
well depending on the type of filter being added. For such a device ethtool 
doesn't know enough to handle RX_CLS_LOC_UNSPEC correctly.

I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is 
enough knowledge to pick an appropriate slot.  So I'd remove the

     if (loc == RX_CLS_LOC_UNSPEC)

block above, let the driver pick a slot, and then pass the selected location 
back for ethtool to report.