netdev - Re: [RFC/RFT] mac80211: implement fq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 01 Mar 2016 15:02:51 +0100
From:	Johannes Berg <johannes@...solutions.net>
To:	Michal Kazior <michal.kazior@...to.com>,
	linux-wireless@...r.kernel.org
Cc:	netdev@...r.kernel.org, eric.dumazet@...il.com,
	dave.taht@...il.com, emmanuel.grumbach@...el.com, nbd@...nwrt.org,
	Tim Shepard <shep@...m.mit.edu>
Subject: Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

On Fri, 2016-02-26 at 14:09 +0100, Michal Kazior wrote:
> 
> +typedef u64 codel_time_t;

Do we really need this? And if yes, does it have to be in the public
header file? Why a typedef anyway?

> - * @txq_ac_max_pending: maximum number of frames per AC pending in
> all txq
> - *	entries for a vif.
> + * @txq_cparams: codel parameters to control tx queueing dropping
> behavior
> + * @txq_limit: maximum number of frames queuesd

typo - queued

> @@ -2133,7 +2155,8 @@ struct ieee80211_hw {
>  	u8 uapsd_max_sp_len;
>  	u8 n_cipher_schemes;
>  	const struct ieee80211_cipher_scheme *cipher_schemes;
> -	int txq_ac_max_pending;
> +	struct codel_params txq_cparams;

Should this really be a parameter the driver determines?

> +static void ieee80211_if_setup_no_queue(struct net_device *dev)
> +{
> +	ieee80211_if_setup(dev);
> +	dev->priv_flags |= IFF_NO_QUEUE;
> +	/* Note for backporters: use dev->tx_queue_len = 0 instead
> of IFF_ */

Heh. Remove that comment; we have an spatch in backports already :)

> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -19,6 +19,7 @@
>  #include <linux/etherdevice.h>
>  #include <linux/rhashtable.h>
>  #include "key.h"
> +#include "codel_i.h"
>  
>  /**
>   * enum ieee80211_sta_info_flags - Stations flags
> @@ -327,6 +328,32 @@ struct mesh_sta {
>  
>  DECLARE_EWMA(signal, 1024, 8)
>  
> +struct txq_info;
> +
> +/**
> + * struct txq_flow - per traffic flow queue
> + *
> + * This structure is used to distinguish and queue different traffic
> flows
> + * separately for fair queueing/AQM purposes.
> + *
> + * @txqi: txq_info structure it is associated at given time
> + * @flowchain: can be linked to other flows for RR purposes
> + * @backlogchain: can be linked to other flows for backlog sorting
> purposes
> + * @queue: sk_buff queue
> + * @cvars: codel state vars
> + * @backlog: number of bytes pending in the queue
> + * @deficit: used for fair queueing balancing
> + */
> +struct txq_flow {
> +	struct txq_info *txqi;
> +	struct list_head flowchain;
> +	struct list_head backlogchain;
> +	struct sk_buff_head queue;
> +	struct codel_vars cvars;
> +	u32 backlog;
> +	u32 deficit;
> +};
> +
>  /**
>   * struct sta_info - STA information
>   *
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index af584f7cdd63..f42f898cb8b5 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -34,6 +34,7 @@
>  #include "wpa.h"
>  #include "wme.h"
>  #include "rate.h"
> +#include "codel.h"

> +static inline codel_time_t
> +custom_codel_get_enqueue_time(struct sk_buff *skb)

There are a lot of inlines here - first of all, do they all need to be
inline?

And secondly, perhaps it would make some sense to move this into
another file?

johannes