[PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Roman Mashak]

Userland client should be able to read an event, and reflect it back to
the kernel, therefore it needs to extract complete set of netlink flags.
    
For example, this will allow "tc monitor" to distinguish Add and Replace
qdisc operations.

Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
 net/sched/sch_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index a9ac912..e3e29be 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -859,7 +859,7 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
 	}
 	if (new && !tc_qdisc_dump_ignore(new, false)) {
 		if (tc_fill_qdisc(skb, new, clid, portid, n->nlmsg_seq,
-				  old ? NLM_F_REPLACE : 0, RTM_NEWQDISC) < 0)
+				  n->nlmsg_flags, RTM_NEWQDISC) < 0)
 			goto err_out;
 	}
 
-- 
1.9.1

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Cong Wang]

On Thu, Oct 26, 2017 at 2:40 PM, Roman Mashak <mrv@mojatatu.com> wrote:
> Userland client should be able to read an event, and reflect it back to
> the kernel, therefore it needs to extract complete set of netlink flags.
>
> For example, this will allow "tc monitor" to distinguish Add and Replace
> qdisc operations.
>
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
> ---
>  net/sched/sch_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index a9ac912..e3e29be 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -859,7 +859,7 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
>         }
>         if (new && !tc_qdisc_dump_ignore(new, false)) {
>                 if (tc_fill_qdisc(skb, new, clid, portid, n->nlmsg_seq,
> -                                 old ? NLM_F_REPLACE : 0, RTM_NEWQDISC) < 0)
> +                                 n->nlmsg_flags, RTM_NEWQDISC) < 0)


Don't you want to change the other tc_fill_qdisc() in the same function
too? ;)

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Roman Mashak]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Thu, Oct 26, 2017 at 2:40 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Userland client should be able to read an event, and reflect it back to
>> the kernel, therefore it needs to extract complete set of netlink flags.
>>
>> For example, this will allow "tc monitor" to distinguish Add and Replace
>> qdisc operations.
>>
>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>> ---
>>  net/sched/sch_api.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index a9ac912..e3e29be 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -859,7 +859,7 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
>>         }
>>         if (new && !tc_qdisc_dump_ignore(new, false)) {
>>                 if (tc_fill_qdisc(skb, new, clid, portid, n->nlmsg_seq,
>> -                                 old ? NLM_F_REPLACE : 0, RTM_NEWQDISC) < 0)
>> +                                 n->nlmsg_flags, RTM_NEWQDISC) < 0)
>
>
> Don't you want to change the other tc_fill_qdisc() in the same function
> too? ;)

The other tc_fill_qdisc generates RTM_DELQDISC event, in that case
netlink flags for user don't matter.

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Cong Wang]

On Sat, Oct 28, 2017 at 8:36 PM, Roman Mashak <mrv@mojatatu.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> writes:
>
>> On Thu, Oct 26, 2017 at 2:40 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>> Userland client should be able to read an event, and reflect it back to
>>> the kernel, therefore it needs to extract complete set of netlink flags.
>>>
>>> For example, this will allow "tc monitor" to distinguish Add and Replace
>>> qdisc operations.
>>>
>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>> ---
>>>  net/sched/sch_api.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>>> index a9ac912..e3e29be 100644
>>> --- a/net/sched/sch_api.c
>>> +++ b/net/sched/sch_api.c
>>> @@ -859,7 +859,7 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
>>>         }
>>>         if (new && !tc_qdisc_dump_ignore(new, false)) {
>>>                 if (tc_fill_qdisc(skb, new, clid, portid, n->nlmsg_seq,
>>> -                                 old ? NLM_F_REPLACE : 0, RTM_NEWQDISC) < 0)
>>> +                                 n->nlmsg_flags, RTM_NEWQDISC) < 0)
>>
>>
>> Don't you want to change the other tc_fill_qdisc() in the same function
>> too? ;)
>
> The other tc_fill_qdisc generates RTM_DELQDISC event, in that case
> netlink flags for user don't matter.

Hmm, I thought you use RTM_NEWQDISC+RTM_DELQDISC to
determine it is replacement, no?

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Roman Mashak]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Sat, Oct 28, 2017 at 8:36 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>
>>> On Thu, Oct 26, 2017 at 2:40 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>> Userland client should be able to read an event, and reflect it back to
>>>> the kernel, therefore it needs to extract complete set of netlink flags.
>>>>
>>>> For example, this will allow "tc monitor" to distinguish Add and Replace
>>>> qdisc operations.
>>>>
>>>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>>>> ---
>>>>  net/sched/sch_api.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>>>> index a9ac912..e3e29be 100644
>>>> --- a/net/sched/sch_api.c
>>>> +++ b/net/sched/sch_api.c
>>>> @@ -859,7 +859,7 @@ static int qdisc_notify(struct net *net, struct sk_buff *oskb,
>>>>         }
>>>>         if (new && !tc_qdisc_dump_ignore(new, false)) {
>>>>                 if (tc_fill_qdisc(skb, new, clid, portid, n->nlmsg_seq,
>>>> -                                 old ? NLM_F_REPLACE : 0, RTM_NEWQDISC) < 0)
>>>> +                                 n->nlmsg_flags, RTM_NEWQDISC) < 0)
>>>
>>>
>>> Don't you want to change the other tc_fill_qdisc() in the same function
>>> too? ;)
>>
>> The other tc_fill_qdisc generates RTM_DELQDISC event, in that case
>> netlink flags for user don't matter.
>
> Hmm, I thought you use RTM_NEWQDISC+RTM_DELQDISC to
> determine it is replacement, no?

Create is RTM_NEWQDISC and NLM_F_EXCL|NLM_F_CREATE, replacement is
RTM_NEWQDISC and NLM_F_REPLACE in netlink flags.

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Cong Wang]

On Mon, Oct 30, 2017 at 11:07 AM, Roman Mashak <mrv@mojatatu.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> writes:
>
>> On Sat, Oct 28, 2017 at 8:36 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>
>> Hmm, I thought you use RTM_NEWQDISC+RTM_DELQDISC to
>> determine it is replacement, no?
>
> Create is RTM_NEWQDISC and NLM_F_EXCL|NLM_F_CREATE, replacement is
> RTM_NEWQDISC and NLM_F_REPLACE in netlink flags.

Is there any reason we can't use RTM_NEWQDISC+RTM_DELQDISC
rather than NLM_F_REPLACE to determine it is replacement?

Note, RTM_NEWQDISC+RTM_DELQDISC are put in a same
message not two.

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Roman Mashak]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Mon, Oct 30, 2017 at 11:07 AM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>
>>> On Sat, Oct 28, 2017 at 8:36 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>>
>>> Hmm, I thought you use RTM_NEWQDISC+RTM_DELQDISC to
>>> determine it is replacement, no?
>>
>> Create is RTM_NEWQDISC and NLM_F_EXCL|NLM_F_CREATE, replacement is
>> RTM_NEWQDISC and NLM_F_REPLACE in netlink flags.
>
> Is there any reason we can't use RTM_NEWQDISC+RTM_DELQDISC
> rather than NLM_F_REPLACE to determine it is replacement?
>

I'm not sure this would be valid semantics for replace operation, look at
the rfc3549:

Additional flag bits for NEW requests
          NLM_F_REPLACE   Replace existing matching config object with
                          this request.

> Note, RTM_NEWQDISC+RTM_DELQDISC are put in a same
> message not two.

Hmm, could you clarify how do you expect to put two event IDs in nlmsg_type?

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Cong Wang]

On Mon, Oct 30, 2017 at 2:17 PM, Roman Mashak <mrv@mojatatu.com> wrote:
> Cong Wang <xiyou.wangcong@gmail.com> writes:
>
>> On Mon, Oct 30, 2017 at 11:07 AM, Roman Mashak <mrv@mojatatu.com> wrote:
>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>>
>>>> On Sat, Oct 28, 2017 at 8:36 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>>>
>>>> Hmm, I thought you use RTM_NEWQDISC+RTM_DELQDISC to
>>>> determine it is replacement, no?
>>>
>>> Create is RTM_NEWQDISC and NLM_F_EXCL|NLM_F_CREATE, replacement is
>>> RTM_NEWQDISC and NLM_F_REPLACE in netlink flags.
>>
>> Is there any reason we can't use RTM_NEWQDISC+RTM_DELQDISC
>> rather than NLM_F_REPLACE to determine it is replacement?
>>
>
> I'm not sure this would be valid semantics for replace operation, look at
> the rfc3549:
>
> Additional flag bits for NEW requests
>           NLM_F_REPLACE   Replace existing matching config object with
>                           this request.
>

I am not saying NLM_F_REPLACE is not correct, I am saying the
RTM_NEWQDISC+RTM_DELQDISC in a same message probably
exists for a reason.


>> Note, RTM_NEWQDISC+RTM_DELQDISC are put in a same
>> message not two.
>
> Hmm, could you clarify how do you expect to put two event IDs in nlmsg_type?

Looking at qdisc_notify(), it is essentially two skb_put() on a same
skb, right? So two nlmsghdr in one skb? Or I read it wrong?

Re: [PATCH net-next 1/1] net sched qdisc: pass netlink message flags in event notification

[Roman Mashak]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Mon, Oct 30, 2017 at 2:17 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>
>>> On Mon, Oct 30, 2017 at 11:07 AM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>>>
>>>>> On Sat, Oct 28, 2017 at 8:36 PM, Roman Mashak <mrv@mojatatu.com> wrote:
>>>>>> Cong Wang <xiyou.wangcong@gmail.com> writes:
>>>>>
>>>>> Hmm, I thought you use RTM_NEWQDISC+RTM_DELQDISC to
>>>>> determine it is replacement, no?
>>>>
>>>> Create is RTM_NEWQDISC and NLM_F_EXCL|NLM_F_CREATE, replacement is
>>>> RTM_NEWQDISC and NLM_F_REPLACE in netlink flags.
>>>
>>> Is there any reason we can't use RTM_NEWQDISC+RTM_DELQDISC
>>> rather than NLM_F_REPLACE to determine it is replacement?
>>>
>>
>> I'm not sure this would be valid semantics for replace operation, look at
>> the rfc3549:
>>
>> Additional flag bits for NEW requests
>>           NLM_F_REPLACE   Replace existing matching config object with
>>                           this request.
>>
>
> I am not saying NLM_F_REPLACE is not correct, I am saying the
> RTM_NEWQDISC+RTM_DELQDISC in a same message probably
> exists for a reason.
>
>
>>> Note, RTM_NEWQDISC+RTM_DELQDISC are put in a same
>>> message not two.
>>
>> Hmm, could you clarify how do you expect to put two event IDs in nlmsg_type?
>
> Looking at qdisc_notify(), it is essentially two skb_put() on a same
> skb, right? So two nlmsghdr in one skb? Or I read it wrong?

So there will be two netlink messages in a single skb, and the user
receives two events. But apparently this only happens when a new
_egress_ qdisc is being added and the default egress qdisc is deleted.