Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: Denis Kenzior <denkenz@gmail.com>, linux-wireless@vger.kernel.org
Subject: Re: [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg
Date: Wed, 11 Sep 2019 11:44:00 +0200
Message-ID: <4397b67b63d1b1a332afa9010e7f48abd54b49b2.camel@sipsolutions.net> (raw)
In-Reply-To: <20190906154303.9303-2-denkenz@gmail.com>

Hi,

The first patch looks good, couple of nits/comments on this one below.

On Fri, 2019-09-06 at 10:43 -0500, Denis Kenzior wrote:
> For historical reasons, NEW_WIPHY messages generated by dumps or
> GET_WIPHY commands were limited to 4096 bytes.  This was due to the
> fact that the kernel only allocated 4k buffers prior to commit
> 9063e21fb026 ("netlink: autosize skb lengthes").  Once the sizes of
> NEW_WIPHY messages exceeded these sizes, split dumps were introduced.

Actually, userspace prior to around the same time *also* only used 4k
buffers (old libnl), and so even with that kernel we still could
possibly have to deal with userspace that had 4k messages only ... but
we could have solved that part trivially instead of adding code to split
it, just the kernel part was still in the way then.

Anyway, I can reword this per my understanding (but will have to reread
all my messages I guess).

>   findings.  E.g. the kernel was at fault for the 4096 byte buffer size
>   limits and not userspace.

Nit: I think most of the time when you write "e.g." ("exempli gratia",
"for example") you really mean "i.e." ("id est", "which is").

> - The code in case '3' is quite complex, but it does try to support a
>   message running out of room in the middle of a channel dump and
>   restarting from where it left off in the next split message.  Perhaps
>   this can be simplified, but it seems this capability is useful.
>   Please take extra care when reviewing this.

Is it useful? You say it basically all fits today, and that means the
channels will either fit into a single message or not ... Then again, if
we add a lot of channels or a lot more data to each channel. Hmm. OK, I
guess better if we do have it.


> +	void *last_good_pos = 0;

Use NULL.

> +		last_good_pos = nlmsg_get_pos(msg);
>  		state->split_start++;

Maybe we're better off having a local macro for these two lines? That
way, we don't risk updating one without the other, which would be
confusing.

> @@ -2004,81 +2004,78 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
>  		if (!nl_bands)
>  			goto nla_put_failure;
>  
> -		for (band = state->band_start;
> -		     band < NUM_NL80211_BANDS; band++) {
> +		/* Position in the buffer if we added a set of channel info */
> +		last_channel_pos = 0;

NULL

> [snip]

> +chan_put_failure:
> +			if (!last_channel_pos)
> +				goto nla_put_failure;
> +
> +			nlmsg_trim(msg, last_channel_pos);
> +			nla_nest_end(msg, nl_freqs);
>  			nla_nest_end(msg, nl_band);
>  
> -			if (state->split) {
> -				/* start again here */
> -				if (state->chan_start)
> -					band--;
> +			if (state->chan_start < sband->n_channels)
>  				break;
> -			}
> +
> +			state->chan_start = 0;
> +			state->band_start++;
>  		}
> -		nla_nest_end(msg, nl_bands);
>  
> -		if (band < NUM_NL80211_BANDS)
> -			state->band_start = band + 1;
> -		else
> -			state->band_start = 0;
> +band_put_failure:
> +		if (!last_channel_pos)
> +			goto nla_put_failure;
> +
> +		nla_nest_end(msg, nl_bands);
>  
> -		/* if bands & channels are done, continue outside */
> -		if (state->band_start == 0 && state->chan_start == 0)
> -			state->split_start++;
> -		if (state->split)
> +		if (state->band_start < NUM_NL80211_BANDS)
>  			break;

Thinking out loud, maybe we could simplify this by just having a small
"stack" of nested attributes to end?

I mean, essentially, you have here similar code to the nla_put_failure
label, in that it finishes and sends out the message, except here you
have to end a bunch of nested attributes.

What if we did something like

#define dump_nest_start(msg, attr) ({ 				\
	struct nlattr r = nla_nest_start_noflag(msg, attr);	\
	BUG_ON(nest_stack_depth >= ARRAY_SIZE(nest_stack);	\
	nest_stack[nest_stack_depth++] = r;			\
	r;							\
})

#define dump_nest_end(msg, r) do {				\
	BUG_ON(nest_stack_depth > 0);				\
	nest_stack_depth--;					\
	BUG_ON(nest_stack[nest_stack_depth] == r);		\
	nla_nest_end(msg, r);					\
} while (0)


or something like that (we probably don't want to use
nla_nest_start_noflag() for future attributes, etc. but anyway).

Then we could unwind any nesting at the end in the common code at the
nla_put_failure label very easily, I think?

> +		 * applications that are not legacy, e.g. ones that requested

i.e. :)

johannes


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-06 15:43 [RFCv3 1/3] nl80211: Fix broken non-split wiphy dumps Denis Kenzior
2019-09-06 15:43 ` [RFCv3 2/3] nl80211: Support >4096 byte NEW_WIPHY event nlmsg Denis Kenzior
2019-09-11  9:44   ` Johannes Berg [this message]
2019-09-11 12:41     ` Denis Kenzior
2019-09-11 15:28       ` Johannes Berg
2019-09-11 13:51         ` Denis Kenzior
2019-10-01  9:23           ` Johannes Berg
2019-09-06 15:43 ` [RFCv3 3/3] nl80211: Send large new_wiphy events Denis Kenzior
2019-09-11  9:47   ` Johannes Berg
2019-09-11 12:20     ` Denis Kenzior
2019-09-11 15:12       ` Johannes Berg
2019-09-11 13:23         ` Denis Kenzior

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4397b67b63d1b1a332afa9010e7f48abd54b49b2.camel@sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=denkenz@gmail.com \
    --cc=linux-wireless@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/ public-inbox