Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[Jiri Pirko]

Mon, Jan 15, 2018 at 06:44:41PM CET, dsah...@gmail.com wrote:
>On 1/15/18 10:27 AM, Jiri Pirko wrote:
>> Mon, Jan 15, 2018 at 06:21:44PM CET, dsah...@gmail.com wrote:
>>> On 1/15/18 10:08 AM, David Ahern wrote:
 On 1/15/18 10:03 AM, Jiri Pirko wrote:
> Mon, Jan 15, 2018 at 05:56:31PM CET, dsah...@gmail.com wrote:
>> On 1/12/18 8:46 AM, Jiri Pirko wrote:
>>> From: Jiri Pirko 
>>
>> Why can't this be done with RTM_GETQDISC?
>
> I don't follow. Could you please describe a bit more what do you think?

 Why are you adding RTM_{NEW,GET,DEL}BLOCK? Can't you get the same
 information using RTM_GETQDISC and updating it to check for the
 'tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK' path
>> 
>> I might, but it bould be an ugly hack. I would use cmd that is used to
>> manipulate qdisc to some entirely different purpose. That does not make
>> any sense to me :(
>> 
>> 
>> 

>>>
>>> The above question is because a user specifies a shared block in a
>>> 'qdisc add'.
>> 
>> Qdisc and block is a different entity
>> 
>> 
>>>
>>> Alternatively, what about RTM_GETTFILTER? You already update
>>> tc_ctl_tfilter to check for TCM_IFINDEX_MAGIC_BLOCK
>> 
>> The object is still filter! Only the handle is different. You cannot
>> compare that, sorry.
>> 
>> 
>>>
>>> My main question is why can't existing RTM_ commands be used?
>
>What I am struggling with is the idea that you need a new set of RTM_
>commands to see if a block exists or to get notifications of a change to
>a block, but you don't need that API to create or modify the blocks.

There is no modify. Create and destroy is done implicitly. That is the
same as for the chains.

I was thinking about how to get info if block exists or not. One way was
new set of rtms that would be also usable for notifications. If we
don't need notifications, it can be done by listing all qdiscs and see
if they don't reference the block. That was not originally possible
because the block info was per-qdisc. Now, per Roopa's suggestion it is
generic.

Okay. I will use qdisc dump for checking block existence. I guess that
block create/destroy notifications are not that useful anyway. We don't
have them for chains either.

Thanks.

Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[David Ahern]

On 1/15/18 10:27 AM, Jiri Pirko wrote:
> Mon, Jan 15, 2018 at 06:21:44PM CET, dsah...@gmail.com wrote:
>> On 1/15/18 10:08 AM, David Ahern wrote:
>>> On 1/15/18 10:03 AM, Jiri Pirko wrote:
 Mon, Jan 15, 2018 at 05:56:31PM CET, dsah...@gmail.com wrote:
> On 1/12/18 8:46 AM, Jiri Pirko wrote:
>> From: Jiri Pirko 
>
> Why can't this be done with RTM_GETQDISC?

 I don't follow. Could you please describe a bit more what do you think?
>>>
>>> Why are you adding RTM_{NEW,GET,DEL}BLOCK? Can't you get the same
>>> information using RTM_GETQDISC and updating it to check for the
>>> 'tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK' path
> 
> I might, but it bould be an ugly hack. I would use cmd that is used to
> manipulate qdisc to some entirely different purpose. That does not make
> any sense to me :(
> 
> 
> 
>>>
>>
>> The above question is because a user specifies a shared block in a
>> 'qdisc add'.
> 
> Qdisc and block is a different entity
> 
> 
>>
>> Alternatively, what about RTM_GETTFILTER? You already update
>> tc_ctl_tfilter to check for TCM_IFINDEX_MAGIC_BLOCK
> 
> The object is still filter! Only the handle is different. You cannot
> compare that, sorry.
> 
> 
>>
>> My main question is why can't existing RTM_ commands be used?

What I am struggling with is the idea that you need a new set of RTM_
commands to see if a block exists or to get notifications of a change to
a block, but you don't need that API to create or modify the blocks.

Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[Jiri Pirko]

Mon, Jan 15, 2018 at 06:21:44PM CET, dsah...@gmail.com wrote:
>On 1/15/18 10:08 AM, David Ahern wrote:
>> On 1/15/18 10:03 AM, Jiri Pirko wrote:
>>> Mon, Jan 15, 2018 at 05:56:31PM CET, dsah...@gmail.com wrote:
 On 1/12/18 8:46 AM, Jiri Pirko wrote:
> From: Jiri Pirko 
>
> Add simple block get operation which primary purpose is to check the
> block existence by block index.
>
> Signed-off-by: Jiri Pirko 
> ---
> v6->v7:
> - new patch
> ---
>  include/uapi/linux/rtnetlink.h |  6 
>  net/sched/cls_api.c| 64 
> ++
>  security/selinux/nlmsgtab.c|  5 +++-
>  3 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/rtnetlink.h 
> b/include/uapi/linux/rtnetlink.h
> index da878f2..4b1f626 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -150,6 +150,12 @@ enum {
>   RTM_NEWCACHEREPORT = 96,
>  #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT
>  
> + RTM_NEWBLOCK = 100,
> +#define RTM_NEWBLOCK RTM_NEWBLOCK
> + RTM_DELBLOCK,
> +#define RTM_DELBLOCK RTM_DELBLOCK
> + RTM_GETBLOCK,
> +#define RTM_GETBLOCK RTM_GETBLOCK
>   __RTM_MAX,
>  #define RTM_MAX  (((__RTM_MAX + 3) & ~3) - 1)
>  };
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d687e58..14e4f20 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1553,6 +1553,69 @@ int tc_setup_cb_call(struct tcf_block *block, 
> struct tcf_exts *exts,
>  }
>  EXPORT_SYMBOL(tc_setup_cb_call);
>  
> +static int block_notify_fill(struct net *net, struct sk_buff *skb,
> +  struct tcf_block *block, u32 portid, u32 seq,
> +  u16 flags, int event)
> +{
> + struct nlmsghdr *nlh;
> + struct tcmsg *tcm;
> +
> + nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
> + if (!nlh)
> + return -EMSGSIZE;
> + tcm = nlmsg_data(nlh);
> + memset(tcm, 0, sizeof(*tcm));
> + tcm->tcm_family = AF_UNSPEC;
> + tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
> + tcm->tcm_block_index = block->index;
> + return 0;
> +}

 Why can't this be done with RTM_GETQDISC?
>>>
>>> I don't follow. Could you please describe a bit more what do you think?
>> 
>> Why are you adding RTM_{NEW,GET,DEL}BLOCK? Can't you get the same
>> information using RTM_GETQDISC and updating it to check for the
>> 'tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK' path

I might, but it bould be an ugly hack. I would use cmd that is used to
manipulate qdisc to some entirely different purpose. That does not make
any sense to me :(



>> 
>
>The above question is because a user specifies a shared block in a
>'qdisc add'.

Qdisc and block is a different entity


>
>Alternatively, what about RTM_GETTFILTER? You already update
>tc_ctl_tfilter to check for TCM_IFINDEX_MAGIC_BLOCK

The object is still filter! Only the handle is different. You cannot
compare that, sorry.


>
>My main question is why can't existing RTM_ commands be used?

Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[David Ahern]

On 1/15/18 10:08 AM, David Ahern wrote:
> On 1/15/18 10:03 AM, Jiri Pirko wrote:
>> Mon, Jan 15, 2018 at 05:56:31PM CET, dsah...@gmail.com wrote:
>>> On 1/12/18 8:46 AM, Jiri Pirko wrote:
 From: Jiri Pirko 

 Add simple block get operation which primary purpose is to check the
 block existence by block index.

 Signed-off-by: Jiri Pirko 
 ---
 v6->v7:
 - new patch
 ---
  include/uapi/linux/rtnetlink.h |  6 
  net/sched/cls_api.c| 64 
 ++
  security/selinux/nlmsgtab.c|  5 +++-
  3 files changed, 74 insertions(+), 1 deletion(-)

 diff --git a/include/uapi/linux/rtnetlink.h 
 b/include/uapi/linux/rtnetlink.h
 index da878f2..4b1f626 100644
 --- a/include/uapi/linux/rtnetlink.h
 +++ b/include/uapi/linux/rtnetlink.h
 @@ -150,6 +150,12 @@ enum {
RTM_NEWCACHEREPORT = 96,
  #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT
  
 +  RTM_NEWBLOCK = 100,
 +#define RTM_NEWBLOCK RTM_NEWBLOCK
 +  RTM_DELBLOCK,
 +#define RTM_DELBLOCK RTM_DELBLOCK
 +  RTM_GETBLOCK,
 +#define RTM_GETBLOCK RTM_GETBLOCK
__RTM_MAX,
  #define RTM_MAX   (((__RTM_MAX + 3) & ~3) - 1)
  };
 diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
 index d687e58..14e4f20 100644
 --- a/net/sched/cls_api.c
 +++ b/net/sched/cls_api.c
 @@ -1553,6 +1553,69 @@ int tc_setup_cb_call(struct tcf_block *block, 
 struct tcf_exts *exts,
  }
  EXPORT_SYMBOL(tc_setup_cb_call);
  
 +static int block_notify_fill(struct net *net, struct sk_buff *skb,
 +   struct tcf_block *block, u32 portid, u32 seq,
 +   u16 flags, int event)
 +{
 +  struct nlmsghdr *nlh;
 +  struct tcmsg *tcm;
 +
 +  nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
 +  if (!nlh)
 +  return -EMSGSIZE;
 +  tcm = nlmsg_data(nlh);
 +  memset(tcm, 0, sizeof(*tcm));
 +  tcm->tcm_family = AF_UNSPEC;
 +  tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
 +  tcm->tcm_block_index = block->index;
 +  return 0;
 +}
>>>
>>> Why can't this be done with RTM_GETQDISC?
>>
>> I don't follow. Could you please describe a bit more what do you think?
> 
> Why are you adding RTM_{NEW,GET,DEL}BLOCK? Can't you get the same
> information using RTM_GETQDISC and updating it to check for the
> 'tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK' path
> 

The above question is because a user specifies a shared block in a
'qdisc add'.

Alternatively, what about RTM_GETTFILTER? You already update
tc_ctl_tfilter to check for TCM_IFINDEX_MAGIC_BLOCK

My main question is why can't existing RTM_ commands be used?

Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[David Ahern]

On 1/15/18 10:03 AM, Jiri Pirko wrote:
> Mon, Jan 15, 2018 at 05:56:31PM CET, dsah...@gmail.com wrote:
>> On 1/12/18 8:46 AM, Jiri Pirko wrote:
>>> From: Jiri Pirko 
>>>
>>> Add simple block get operation which primary purpose is to check the
>>> block existence by block index.
>>>
>>> Signed-off-by: Jiri Pirko 
>>> ---
>>> v6->v7:
>>> - new patch
>>> ---
>>>  include/uapi/linux/rtnetlink.h |  6 
>>>  net/sched/cls_api.c| 64 
>>> ++
>>>  security/selinux/nlmsgtab.c|  5 +++-
>>>  3 files changed, 74 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>>> index da878f2..4b1f626 100644
>>> --- a/include/uapi/linux/rtnetlink.h
>>> +++ b/include/uapi/linux/rtnetlink.h
>>> @@ -150,6 +150,12 @@ enum {
>>> RTM_NEWCACHEREPORT = 96,
>>>  #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT
>>>  
>>> +   RTM_NEWBLOCK = 100,
>>> +#define RTM_NEWBLOCK RTM_NEWBLOCK
>>> +   RTM_DELBLOCK,
>>> +#define RTM_DELBLOCK RTM_DELBLOCK
>>> +   RTM_GETBLOCK,
>>> +#define RTM_GETBLOCK RTM_GETBLOCK
>>> __RTM_MAX,
>>>  #define RTM_MAX(((__RTM_MAX + 3) & ~3) - 1)
>>>  };
>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>> index d687e58..14e4f20 100644
>>> --- a/net/sched/cls_api.c
>>> +++ b/net/sched/cls_api.c
>>> @@ -1553,6 +1553,69 @@ int tc_setup_cb_call(struct tcf_block *block, struct 
>>> tcf_exts *exts,
>>>  }
>>>  EXPORT_SYMBOL(tc_setup_cb_call);
>>>  
>>> +static int block_notify_fill(struct net *net, struct sk_buff *skb,
>>> +struct tcf_block *block, u32 portid, u32 seq,
>>> +u16 flags, int event)
>>> +{
>>> +   struct nlmsghdr *nlh;
>>> +   struct tcmsg *tcm;
>>> +
>>> +   nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
>>> +   if (!nlh)
>>> +   return -EMSGSIZE;
>>> +   tcm = nlmsg_data(nlh);
>>> +   memset(tcm, 0, sizeof(*tcm));
>>> +   tcm->tcm_family = AF_UNSPEC;
>>> +   tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
>>> +   tcm->tcm_block_index = block->index;
>>> +   return 0;
>>> +}
>>
>> Why can't this be done with RTM_GETQDISC?
> 
> I don't follow. Could you please describe a bit more what do you think?

Why are you adding RTM_{NEW,GET,DEL}BLOCK? Can't you get the same
information using RTM_GETQDISC and updating it to check for the
'tcm_ifindex == TCM_IFINDEX_MAGIC_BLOCK' path

Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[Jiri Pirko]

Mon, Jan 15, 2018 at 05:56:31PM CET, dsah...@gmail.com wrote:
>On 1/12/18 8:46 AM, Jiri Pirko wrote:
>> From: Jiri Pirko 
>> 
>> Add simple block get operation which primary purpose is to check the
>> block existence by block index.
>> 
>> Signed-off-by: Jiri Pirko 
>> ---
>> v6->v7:
>> - new patch
>> ---
>>  include/uapi/linux/rtnetlink.h |  6 
>>  net/sched/cls_api.c| 64 
>> ++
>>  security/selinux/nlmsgtab.c|  5 +++-
>>  3 files changed, 74 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index da878f2..4b1f626 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -150,6 +150,12 @@ enum {
>>  RTM_NEWCACHEREPORT = 96,
>>  #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT
>>  
>> +RTM_NEWBLOCK = 100,
>> +#define RTM_NEWBLOCK RTM_NEWBLOCK
>> +RTM_DELBLOCK,
>> +#define RTM_DELBLOCK RTM_DELBLOCK
>> +RTM_GETBLOCK,
>> +#define RTM_GETBLOCK RTM_GETBLOCK
>>  __RTM_MAX,
>>  #define RTM_MAX (((__RTM_MAX + 3) & ~3) - 1)
>>  };
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index d687e58..14e4f20 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -1553,6 +1553,69 @@ int tc_setup_cb_call(struct tcf_block *block, struct 
>> tcf_exts *exts,
>>  }
>>  EXPORT_SYMBOL(tc_setup_cb_call);
>>  
>> +static int block_notify_fill(struct net *net, struct sk_buff *skb,
>> + struct tcf_block *block, u32 portid, u32 seq,
>> + u16 flags, int event)
>> +{
>> +struct nlmsghdr *nlh;
>> +struct tcmsg *tcm;
>> +
>> +nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
>> +if (!nlh)
>> +return -EMSGSIZE;
>> +tcm = nlmsg_data(nlh);
>> +memset(tcm, 0, sizeof(*tcm));
>> +tcm->tcm_family = AF_UNSPEC;
>> +tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
>> +tcm->tcm_block_index = block->index;
>> +return 0;
>> +}
>
>Why can't this be done with RTM_GETQDISC?

I don't follow. Could you please describe a bit more what do you think?

Thanks!

Re: [patch net-next v8 08/14] net: sched: add rt netlink message type for block get

[David Ahern]

On 1/12/18 8:46 AM, Jiri Pirko wrote:
> From: Jiri Pirko 
> 
> Add simple block get operation which primary purpose is to check the
> block existence by block index.
> 
> Signed-off-by: Jiri Pirko 
> ---
> v6->v7:
> - new patch
> ---
>  include/uapi/linux/rtnetlink.h |  6 
>  net/sched/cls_api.c| 64 
> ++
>  security/selinux/nlmsgtab.c|  5 +++-
>  3 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
> index da878f2..4b1f626 100644
> --- a/include/uapi/linux/rtnetlink.h
> +++ b/include/uapi/linux/rtnetlink.h
> @@ -150,6 +150,12 @@ enum {
>   RTM_NEWCACHEREPORT = 96,
>  #define RTM_NEWCACHEREPORT RTM_NEWCACHEREPORT
>  
> + RTM_NEWBLOCK = 100,
> +#define RTM_NEWBLOCK RTM_NEWBLOCK
> + RTM_DELBLOCK,
> +#define RTM_DELBLOCK RTM_DELBLOCK
> + RTM_GETBLOCK,
> +#define RTM_GETBLOCK RTM_GETBLOCK
>   __RTM_MAX,
>  #define RTM_MAX  (((__RTM_MAX + 3) & ~3) - 1)
>  };
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d687e58..14e4f20 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1553,6 +1553,69 @@ int tc_setup_cb_call(struct tcf_block *block, struct 
> tcf_exts *exts,
>  }
>  EXPORT_SYMBOL(tc_setup_cb_call);
>  
> +static int block_notify_fill(struct net *net, struct sk_buff *skb,
> +  struct tcf_block *block, u32 portid, u32 seq,
> +  u16 flags, int event)
> +{
> + struct nlmsghdr *nlh;
> + struct tcmsg *tcm;
> +
> + nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
> + if (!nlh)
> + return -EMSGSIZE;
> + tcm = nlmsg_data(nlh);
> + memset(tcm, 0, sizeof(*tcm));
> + tcm->tcm_family = AF_UNSPEC;
> + tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
> + tcm->tcm_block_index = block->index;
> + return 0;
> +}

Why can't this be done with RTM_GETQDISC?