[PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This flag doesn't exist on newer kernels, but replaced the tx_queue_len
assignment, so can't just be backported to have no effect. Instead, add
a semantic patch that puts back the tx_queue_len=0 assignment on older
kernel versions.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 patches/collateral-evolutions/network/0062-iff-no-queue.cocci | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 patches/collateral-evolutions/network/0062-iff-no-queue.cocci

diff --git a/patches/collateral-evolutions/network/0062-iff-no-queue.cocci b/patches/collateral-evolutions/network/0062-iff-no-queue.cocci
new file mode 100644
index 000000000000..9c95b853df18
--- /dev/null
+++ b/patches/collateral-evolutions/network/0062-iff-no-queue.cocci
@@ -0,0 +1,9 @@
+@@
+expression E;
+@@
+
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
+ E->priv_flags |= IFF_NO_QUEUE;
++#else
++E->tx_queue_len = 0;
++#endif
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Luis R. Rodriguez]

On Wed, Sep 9, 2015 at 2:27 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> This flag doesn't exist on newer kernels, but replaced the tx_queue_len
> assignment, so can't just be backported to have no effect. Instead, add
> a semantic patch that puts back the tx_queue_len=0 assignment on older
> kernel versions.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  patches/collateral-evolutions/network/0062-iff-no-queue.cocci | 9 +++++++++
>  1 file changed, 9 insertions(+)
>  create mode 100644 patches/collateral-evolutions/network/0062-iff-no-queue.cocci
>
> diff --git a/patches/collateral-evolutions/network/0062-iff-no-queue.cocci b/patches/collateral-evolutions/network/0062-iff-no-queue.cocci
> new file mode 100644
> index 000000000000..9c95b853df18
> --- /dev/null
> +++ b/patches/collateral-evolutions/network/0062-iff-no-queue.cocci
> @@ -0,0 +1,9 @@
> +@@
> +expression E;
> +@@
> +
> ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
> + E->priv_flags |= IFF_NO_QUEUE;
> ++#else
> ++E->tx_queue_len = 0;
> ++#endif

Interesting so although priv_flags may be a member name prevalent in
*many* data structures the SmPL rule here is very specific about the
use of IFF_NO_QUEUE as a flag, and since we know that is unique to one
use case we take the liberty over using expression here. Replying just
to annotate this practice and Cc Julia on her thoughts.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Johannes Berg]

On Mon, 2015-09-14 at 01:25 -0700, Luis R. Rodriguez wrote:

> > ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
> > + E->priv_flags |= IFF_NO_QUEUE;
> > ++#else
> > ++E->tx_queue_len = 0;
> > ++#endif
> 
> Interesting so although priv_flags may be a member name prevalent in
> *many* data structures the SmPL rule here is very specific about the
> use of IFF_NO_QUEUE as a flag, and since we know that is unique to one
> use case we take the liberty over using expression here. Replying just
> to annotate this practice and Cc Julia on her thoughts.
> 

Yeah I thought about this for a while - it doesn't even cover all cases
(there might be drivers that don't use |=, for example).

However, for now this seemed sufficient since very few places in the
code actually use this.

That said, there are cases where E really needs to be an expression
since it's not just "dev->..." but something like "foo->dev->...".

johannes
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Luis R. Rodriguez]

On Mon, Sep 14, 2015 at 1:27 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2015-09-14 at 01:25 -0700, Luis R. Rodriguez wrote:
>
>> > ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
>> > + E->priv_flags |= IFF_NO_QUEUE;
>> > ++#else
>> > ++E->tx_queue_len = 0;
>> > ++#endif
>>
>> Interesting so although priv_flags may be a member name prevalent in
>> *many* data structures the SmPL rule here is very specific about the
>> use of IFF_NO_QUEUE as a flag, and since we know that is unique to one
>> use case we take the liberty over using expression here. Replying just
>> to annotate this practice and Cc Julia on her thoughts.
>>
>
> Yeah I thought about this for a while - it doesn't even cover all cases
> (there might be drivers that don't use |=, for example).
>
> However, for now this seemed sufficient since very few places in the
> code actually use this.

Sure, I wonder we could use some early rule checks to catch some
liberal assumptions we make for some of our rules in patches/, we
would use that to report issues *prior* to patch application and we'd
reject moving forward with generation if all these rule checks don't
match. If we do this that would stop patch application / compilation
testing proactively. Something as with the kernel's
scripts/coccinelle/ 'make coccicheck' prior to running our patch
application. This case seems a bit hard to cover for all cases that
don't use the assignments as in your rule though I think... if its
easy though it could be a good example to tackle.

> That said, there are cases where E really needs to be an expression
> since it's not just "dev->..." but something like "foo->dev->...".

Right figured.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Julia Lawall]

On Mon, 14 Sep 2015, Luis R. Rodriguez wrote:

> On Mon, Sep 14, 2015 at 1:27 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Mon, 2015-09-14 at 01:25 -0700, Luis R. Rodriguez wrote:
> >
> >> > ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
> >> > + E->priv_flags |= IFF_NO_QUEUE;
> >> > ++#else
> >> > ++E->tx_queue_len = 0;
> >> > ++#endif
> >>
> >> Interesting so although priv_flags may be a member name prevalent in
> >> *many* data structures the SmPL rule here is very specific about the
> >> use of IFF_NO_QUEUE as a flag, and since we know that is unique to one
> >> use case we take the liberty over using expression here. Replying just
> >> to annotate this practice and Cc Julia on her thoughts.
> >>
> >
> > Yeah I thought about this for a while - it doesn't even cover all cases
> > (there might be drivers that don't use |=, for example).
> >
> > However, for now this seemed sufficient since very few places in the
> > code actually use this.
> 
> Sure, I wonder we could use some early rule checks to catch some
> liberal assumptions we make for some of our rules in patches/, we
> would use that to report issues *prior* to patch application and we'd
> reject moving forward with generation if all these rule checks don't
> match. If we do this that would stop patch application / compilation
> testing proactively. Something as with the kernel's
> scripts/coccinelle/ 'make coccicheck' prior to running our patch
> application. This case seems a bit hard to cover for all cases that
> don't use the assignments as in your rule though I think... if its
> easy though it could be a good example to tackle.

I'm not sure to understand what you have in mind.  Could you be more
concrete?

julia



> 
> > That said, there are cases where E really needs to be an expression
> > since it's not just "dev->..." but something like "foo->dev->...".
> 
> Right figured.
> 
>  Luis
> 
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Luis R. Rodriguez]

On Mon, Sep 14, 2015 at 12:48 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Mon, 14 Sep 2015, Luis R. Rodriguez wrote:
>
>> On Mon, Sep 14, 2015 at 1:27 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Mon, 2015-09-14 at 01:25 -0700, Luis R. Rodriguez wrote:
>> >
>> >> > ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
>> >> > + E->priv_flags |= IFF_NO_QUEUE;
>> >> > ++#else
>> >> > ++E->tx_queue_len = 0;
>> >> > ++#endif
>> >>
>> >> Interesting so although priv_flags may be a member name prevalent in
>> >> *many* data structures the SmPL rule here is very specific about the
>> >> use of IFF_NO_QUEUE as a flag, and since we know that is unique to one
>> >> use case we take the liberty over using expression here. Replying just
>> >> to annotate this practice and Cc Julia on her thoughts.
>> >>
>> >
>> > Yeah I thought about this for a while - it doesn't even cover all cases
>> > (there might be drivers that don't use |=, for example).
>> >
>> > However, for now this seemed sufficient since very few places in the
>> > code actually use this.
>>
>> Sure, I wonder we could use some early rule checks to catch some
>> liberal assumptions we make for some of our rules in patches/, we
>> would use that to report issues *prior* to patch application and we'd
>> reject moving forward with generation if all these rule checks don't
>> match. If we do this that would stop patch application / compilation
>> testing proactively. Something as with the kernel's
>> scripts/coccinelle/ 'make coccicheck' prior to running our patch
>> application. This case seems a bit hard to cover for all cases that
>> don't use the assignments as in your rule though I think... if its
>> easy though it could be a good example to tackle.
>
> I'm not sure to understand what you have in mind.  Could you be more
> concrete?

The idea was to put in place a grammatical expression of how the code
is expected to be used and nag about when this is false, because when
it is false the semantic patch in place would likely need to be
updated to address new forms. Without such a sanity check we'll only
find out about issues later upon compilation time, and that is
reactive.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Julia Lawall]

On Mon, 14 Sep 2015, Johannes Berg wrote:

> On Mon, 2015-09-14 at 01:25 -0700, Luis R. Rodriguez wrote:
> 
> > > ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,3,0)
> > > + E->priv_flags |= IFF_NO_QUEUE;
> > > ++#else
> > > ++E->tx_queue_len = 0;
> > > ++#endif
> > 
> > Interesting so although priv_flags may be a member name prevalent in
> > *many* data structures the SmPL rule here is very specific about the
> > use of IFF_NO_QUEUE as a flag, and since we know that is unique to one
> > use case we take the liberty over using expression here. Replying just
> > to annotate this practice and Cc Julia on her thoughts.

I'm OK with the above reasoning for using expression.  If you don't use 
expression, you need to be sure that adequate type information is 
available.  So if the context is unambiguous for some reason, then 
expression is better.

julia

> > 
> 
> Yeah I thought about this for a while - it doesn't even cover all cases
> (there might be drivers that don't use |=, for example).
> 
> However, for now this seemed sufficient since very few places in the
> code actually use this.
> 
> That said, there are cases where E really needs to be an expression
> since it's not just "dev->..." but something like "foo->dev->...".
> 
> johannes
> 
--
To unsubscribe from this list: send the line "unsubscribe backports" in

Re: [PATCH v2] backports: add spatch to handle IFF_NO_QUEUE

[Hauke Mehrtens]

On 09/09/2015 11:27 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This flag doesn't exist on newer kernels, but replaced the tx_queue_len
> assignment, so can't just be backported to have no effect. Instead, add
> a semantic patch that puts back the tx_queue_len=0 assignment on older
> kernel versions.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  patches/collateral-evolutions/network/0062-iff-no-queue.cocci | 9 +++++++++
>  1 file changed, 9 insertions(+)
>  create mode 100644 patches/collateral-evolutions/network/0062-iff-no-queue.cocci
> 

Thank you for the patch, it was applied and pushed out.

Hauke
--
To unsubscribe from this list: send the line "unsubscribe backports" in