* [MPTCP] Re: [MPTCP][PATCH mptcp-next 3/5] mptcp: deal with MPTCP_PM_ADDR_FLAG_BACKUP in PM netlink
@ 2020-11-21 1:17 Mat Martineau
0 siblings, 0 replies; 5+ messages in thread
From: Mat Martineau @ 2020-11-21 1:17 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]
On Fri, 20 Nov 2020, Geliang Tang wrote:
> This patch added the MP_PRIO support for PM netlink:
>
> When PM netlink adds an address, check whether the
> MPTCP_PM_ADDR_FLAG_BACKUP flag is set. If it is, check whether this
> address had been added in the local address list. If it had been, then
> call mptcp_nl_addr_backup to deal with this address.
>
> In mptcp_nl_addr_backup, we traverse all the existing msk sockets to find
> the relevant sockets, and get the backup value according to whether the
> MPTCP_PM_ADDR_FLAG_BACKUP flag is set, then call
> mptcp_pm_nl_mp_prio_send_ack to send out a MP_PRIO ACK packet.
>
> In mptcp_pm_nl_mp_prio_send_ack, we set or clear the
> MPTCP_PM_ADDR_FLAG_BACKUP flag.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm_netlink.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index dd48af3c5ed5..58960e1cdc32 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -535,6 +535,11 @@ void mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> return;
> }
>
> + if (addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> + addr->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
> + else
> + addr->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
> +
Why is the backup bit inverted here? It's not clear why it's changed, and
why bkup is passed in based on the value of the bit when read by the
calling function.
> subflow->backup = bkup;
> mptcp_schedule_work(sk);
>
> @@ -847,17 +852,53 @@ static struct pm_nl_pernet *genl_info_pm_nl(struct genl_info *info)
> return net_generic(genl_info_net(info), pm_nl_pernet_id);
> }
>
> +static int mptcp_nl_addr_backup(struct net *net,
> + struct mptcp_addr_info *addr)
> +{
> + long s_slot = 0, s_num = 0;
> + struct mptcp_sock *msk;
> +
> + while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
> + u8 bkup = !(addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> + struct sock *sk = (struct sock *)msk;
> +
> + if (list_empty(&msk->conn_list))
> + goto next;
> +
> + lock_sock(sk);
> + spin_lock_bh(&msk->pm.lock);
> + mptcp_pm_nl_mp_prio_send_ack(msk, addr, bkup);
> + spin_unlock_bh(&msk->pm.lock);
> + release_sock(sk);
> +
> +next:
> + sock_put(sk);
> + cond_resched();
> + }
> +
> + return 0;
> +}
> +
> static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> {
> struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
> struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
> struct mptcp_pm_addr_entry addr, *entry;
> + struct net *net = sock_net(skb->sk);
> int ret;
>
> ret = mptcp_pm_parse_addr(attr, info, true, &addr);
> if (ret < 0)
> return ret;
>
> + if (addr.addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP) {
> + list_for_each_entry(entry, &pernet->local_addr_list, list) {
> + if (addresses_equal(&entry->addr, &addr.addr, false))
> + mptcp_nl_addr_backup(net, &entry->addr);
I think a more strict match should be required here that includes the
'id' as well. There may be multiple endpoints with the same address but
different port numbers.
> + }
> + return 0;
> + }
> +
> entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> if (!entry) {
> GENL_SET_ERR_MSG(info, "can't allocate addr");
> --
> 2.26.2
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH mptcp-next 3/5] mptcp: deal with MPTCP_PM_ADDR_FLAG_BACKUP in PM netlink
@ 2020-11-30 2:37 Geliang Tang
0 siblings, 0 replies; 5+ messages in thread
From: Geliang Tang @ 2020-11-30 2:37 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4897 bytes --]
Hi Mat,
Thanks for your review.
Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年11月21日周六 上午9:17写道：
>
> On Fri, 20 Nov 2020, Geliang Tang wrote:
>
> > This patch added the MP_PRIO support for PM netlink:
> >
> > When PM netlink adds an address, check whether the
> > MPTCP_PM_ADDR_FLAG_BACKUP flag is set. If it is, check whether this
> > address had been added in the local address list. If it had been, then
> > call mptcp_nl_addr_backup to deal with this address.
> >
> > In mptcp_nl_addr_backup, we traverse all the existing msk sockets to find
> > the relevant sockets, and get the backup value according to whether the
> > MPTCP_PM_ADDR_FLAG_BACKUP flag is set, then call
> > mptcp_pm_nl_mp_prio_send_ack to send out a MP_PRIO ACK packet.
> >
> > In mptcp_pm_nl_mp_prio_send_ack, we set or clear the
> > MPTCP_PM_ADDR_FLAG_BACKUP flag.
> >
> > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > ---
> > net/mptcp/pm_netlink.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 41 insertions(+)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index dd48af3c5ed5..58960e1cdc32 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -535,6 +535,11 @@ void mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> > return;
> > }
> >
> > + if (addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> > + addr->flags &= ~MPTCP_PM_ADDR_FLAG_BACKUP;
> > + else
> > + addr->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
> > +
>
> Why is the backup bit inverted here? It's not clear why it's changed, and
> why bkup is passed in based on the value of the bit when read by the
> calling function.
>
I think we can use backup command not only to 'turn on' a subflow's backup
bit, but also to 'turn off' a subflow's backup bit. So the pm_nl_ctl backup
command will work like this:
# pm_nl_ctl add 10.0.2.1 flags signal; pm_nl_ctl dump
id 1 flags signal 10.0.2.1
# pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
id 1 flags signal,backup 10.0.2.1
# pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
id 1 flags signal 10.0.2.1
> > subflow->backup = bkup;
> > mptcp_schedule_work(sk);
> >
> > @@ -847,17 +852,53 @@ static struct pm_nl_pernet *genl_info_pm_nl(struct genl_info *info)
> > return net_generic(genl_info_net(info), pm_nl_pernet_id);
> > }
> >
> > +static int mptcp_nl_addr_backup(struct net *net,
> > + struct mptcp_addr_info *addr)
> > +{
> > + long s_slot = 0, s_num = 0;
> > + struct mptcp_sock *msk;
> > +
> > + while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
> > + u8 bkup = !(addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> > + struct sock *sk = (struct sock *)msk;
> > +
> > + if (list_empty(&msk->conn_list))
> > + goto next;
> > +
> > + lock_sock(sk);
> > + spin_lock_bh(&msk->pm.lock);
> > + mptcp_pm_nl_mp_prio_send_ack(msk, addr, bkup);
> > + spin_unlock_bh(&msk->pm.lock);
> > + release_sock(sk);
> > +
> > +next:
> > + sock_put(sk);
> > + cond_resched();
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> > {
> > struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
> > struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
> > struct mptcp_pm_addr_entry addr, *entry;
> > + struct net *net = sock_net(skb->sk);
> > int ret;
> >
> > ret = mptcp_pm_parse_addr(attr, info, true, &addr);
> > if (ret < 0)
> > return ret;
> >
> > + if (addr.addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP) {
> > + list_for_each_entry(entry, &pernet->local_addr_list, list) {
> > + if (addresses_equal(&entry->addr, &addr.addr, false))
> > + mptcp_nl_addr_backup(net, &entry->addr);
>
> I think a more strict match should be required here that includes the
> 'id' as well. There may be multiple endpoints with the same address but
> different port numbers.
>
Adding 'id' match doesn't work, since addr.addr's id is always 0 here.
I think we can use addresses_equal(&entry->addr, &addr.addr, true) here,
it's enough to cover the same address with different port numbers case.
-Geliang
> > + }
> > + return 0;
> > + }
> > +
> > entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> > if (!entry) {
> > GENL_SET_ERR_MSG(info, "can't allocate addr");
> > --
> > 2.26.2
>
> --
> Mat Martineau
> Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH mptcp-next 3/5] mptcp: deal with MPTCP_PM_ADDR_FLAG_BACKUP in PM netlink
@ 2020-12-02 0:40 Mat Martineau
0 siblings, 0 replies; 5+ messages in thread
From: Mat Martineau @ 2020-12-02 0:40 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 5376 bytes --]
On Mon, 30 Nov 2020, Geliang Tang wrote:
> Hi Mat,
>
> Thanks for your review.
>
> Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年11月21日周六 上午9:17写道：
>>
>> On Fri, 20 Nov 2020, Geliang Tang wrote:
>>
>>> This patch added the MP_PRIO support for PM netlink:
>>>
>>> When PM netlink adds an address, check whether the
>>> MPTCP_PM_ADDR_FLAG_BACKUP flag is set. If it is, check whether this
>>> address had been added in the local address list. If it had been, then
>>> call mptcp_nl_addr_backup to deal with this address.
>>>
>>> In mptcp_nl_addr_backup, we traverse all the existing msk sockets to find
>>> the relevant sockets, and get the backup value according to whether the
>>> MPTCP_PM_ADDR_FLAG_BACKUP flag is set, then call
>>> mptcp_pm_nl_mp_prio_send_ack to send out a MP_PRIO ACK packet.
>>>
>>> In mptcp_pm_nl_mp_prio_send_ack, we set or clear the
>>> MPTCP_PM_ADDR_FLAG_BACKUP flag.
>>>
>>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
>>> ---
>>> net/mptcp/pm_netlink.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 41 insertions(+)
>>>
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index dd48af3c5ed5..58960e1cdc32 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -535,6 +535,11 @@ void mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>>> return;
>>> }
>>>
>>> + if (addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
>>> + addr->flags &= ‾MPTCP_PM_ADDR_FLAG_BACKUP;
>>> + else
>>> + addr->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
>>> +
>>
>> Why is the backup bit inverted here? It's not clear why it's changed, and
>> why bkup is passed in based on the value of the bit when read by the
>> calling function.
>>
>
> I think we can use backup command not only to 'turn on' a subflow's backup
> bit, but also to 'turn off' a subflow's backup bit. So the pm_nl_ctl backup
> command will work like this:
>
> # pm_nl_ctl add 10.0.2.1 flags signal; pm_nl_ctl dump
> id 1 flags signal 10.0.2.1
> # pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
> id 1 flags signal,backup 10.0.2.1
> # pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
> id 1 flags signal 10.0.2.1
I would prefer to have the 'backup' bit in the netlink flags set the
backup bit directly - if the backup flag is 0 the subflow backup bit gets
set to 0, and if the netlink backup flag is 1 the subflow backup bit gets
set to 1. Paolo, does that interpretation of the backup flag match what
you were thinking too?
>
>>> subflow->backup = bkup;
>>> mptcp_schedule_work(sk);
>>>
>>> @@ -847,17 +852,53 @@ static struct pm_nl_pernet *genl_info_pm_nl(struct genl_info *info)
>>> return net_generic(genl_info_net(info), pm_nl_pernet_id);
>>> }
>>>
>>> +static int mptcp_nl_addr_backup(struct net *net,
>>> + struct mptcp_addr_info *addr)
>>> +{
>>> + long s_slot = 0, s_num = 0;
>>> + struct mptcp_sock *msk;
>>> +
>>> + while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
>>> + u8 bkup = !(addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>>> + struct sock *sk = (struct sock *)msk;
>>> +
>>> + if (list_empty(&msk->conn_list))
>>> + goto next;
>>> +
>>> + lock_sock(sk);
>>> + spin_lock_bh(&msk->pm.lock);
>>> + mptcp_pm_nl_mp_prio_send_ack(msk, addr, bkup);
>>> + spin_unlock_bh(&msk->pm.lock);
>>> + release_sock(sk);
>>> +
>>> +next:
>>> + sock_put(sk);
>>> + cond_resched();
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
>>> {
>>> struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
>>> struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
>>> struct mptcp_pm_addr_entry addr, *entry;
>>> + struct net *net = sock_net(skb->sk);
>>> int ret;
>>>
>>> ret = mptcp_pm_parse_addr(attr, info, true, &addr);
>>> if (ret < 0)
>>> return ret;
>>>
>>> + if (addr.addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP) {
>>> + list_for_each_entry(entry, &pernet->local_addr_list, list) {
>>> + if (addresses_equal(&entry->addr, &addr.addr, false))
>>> + mptcp_nl_addr_backup(net, &entry->addr);
>>
>> I think a more strict match should be required here that includes the
>> 'id' as well. There may be multiple endpoints with the same address but
>> different port numbers.
>>
>
> Adding 'id' match doesn't work, since addr.addr's id is always 0 here.
>
> I think we can use addresses_equal(&entry->addr, &addr.addr, true) here,
> it's enough to cover the same address with different port numbers case.
Ok, yes, addresses_equal() does compare port numbers so that should help.
>>> + }
>>> + return 0;
>>> + }
>>> +
>>> entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>>> if (!entry) {
>>> GENL_SET_ERR_MSG(info, "can't allocate addr");
>>> --
>>> 2.26.2
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH mptcp-next 3/5] mptcp: deal with MPTCP_PM_ADDR_FLAG_BACKUP in PM netlink
@ 2020-12-02 4:35 Geliang Tang
0 siblings, 0 replies; 5+ messages in thread
From: Geliang Tang @ 2020-12-02 4:35 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 6153 bytes --]
Hi Mat,
Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年12月2日周三 上午8:40写道：
>
> On Mon, 30 Nov 2020, Geliang Tang wrote:
>
> > Hi Mat,
> >
> > Thanks for your review.
> >
> > Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年11月21日周六 上午9:17写道：
> >>
> >> On Fri, 20 Nov 2020, Geliang Tang wrote:
> >>
> >>> This patch added the MP_PRIO support for PM netlink:
> >>>
> >>> When PM netlink adds an address, check whether the
> >>> MPTCP_PM_ADDR_FLAG_BACKUP flag is set. If it is, check whether this
> >>> address had been added in the local address list. If it had been, then
> >>> call mptcp_nl_addr_backup to deal with this address.
> >>>
> >>> In mptcp_nl_addr_backup, we traverse all the existing msk sockets to find
> >>> the relevant sockets, and get the backup value according to whether the
> >>> MPTCP_PM_ADDR_FLAG_BACKUP flag is set, then call
> >>> mptcp_pm_nl_mp_prio_send_ack to send out a MP_PRIO ACK packet.
> >>>
> >>> In mptcp_pm_nl_mp_prio_send_ack, we set or clear the
> >>> MPTCP_PM_ADDR_FLAG_BACKUP flag.
> >>>
> >>> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> >>> ---
> >>> net/mptcp/pm_netlink.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 41 insertions(+)
> >>>
> >>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> >>> index dd48af3c5ed5..58960e1cdc32 100644
> >>> --- a/net/mptcp/pm_netlink.c
> >>> +++ b/net/mptcp/pm_netlink.c
> >>> @@ -535,6 +535,11 @@ void mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> >>> return;
> >>> }
> >>>
> >>> + if (addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> >>> + addr->flags &= ‾MPTCP_PM_ADDR_FLAG_BACKUP;
> >>> + else
> >>> + addr->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
> >>> +
> >>
> >> Why is the backup bit inverted here? It's not clear why it's changed, and
> >> why bkup is passed in based on the value of the bit when read by the
> >> calling function.
> >>
> >
> > I think we can use backup command not only to 'turn on' a subflow's backup
> > bit, but also to 'turn off' a subflow's backup bit. So the pm_nl_ctl backup
> > command will work like this:
> >
> > # pm_nl_ctl add 10.0.2.1 flags signal; pm_nl_ctl dump
> > id 1 flags signal 10.0.2.1
> > # pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
> > id 1 flags signal,backup 10.0.2.1
> > # pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
> > id 1 flags signal 10.0.2.1
>
> I would prefer to have the 'backup' bit in the netlink flags set the
> backup bit directly - if the backup flag is 0 the subflow backup bit gets
> set to 0, and if the netlink backup flag is 1 the subflow backup bit gets
> set to 1. Paolo, does that interpretation of the backup flag match what
> you were thinking too?
Should we add an argument behind backup for the pm_nl_ctl tool? Then we
can use pm_nl_ctl to set backup flag like this:
pm_nl_ctl add 10.0.2.1 flags backup 1
pm_nl_ctl add 10.0.2.1 flags backup 0
Or add a new argument 'set' instead of using 'add' when changing
the backup flag:
pm_nl_ctl set 10.0.2.1 flags backup 1
pm_nl_ctl set 10.0.2.1 flags backup 0
-Geliang
>
>
> >
> >>> subflow->backup = bkup;
> >>> mptcp_schedule_work(sk);
> >>>
> >>> @@ -847,17 +852,53 @@ static struct pm_nl_pernet *genl_info_pm_nl(struct genl_info *info)
> >>> return net_generic(genl_info_net(info), pm_nl_pernet_id);
> >>> }
> >>>
> >>> +static int mptcp_nl_addr_backup(struct net *net,
> >>> + struct mptcp_addr_info *addr)
> >>> +{
> >>> + long s_slot = 0, s_num = 0;
> >>> + struct mptcp_sock *msk;
> >>> +
> >>> + while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
> >>> + u8 bkup = !(addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> >>> + struct sock *sk = (struct sock *)msk;
> >>> +
> >>> + if (list_empty(&msk->conn_list))
> >>> + goto next;
> >>> +
> >>> + lock_sock(sk);
> >>> + spin_lock_bh(&msk->pm.lock);
> >>> + mptcp_pm_nl_mp_prio_send_ack(msk, addr, bkup);
> >>> + spin_unlock_bh(&msk->pm.lock);
> >>> + release_sock(sk);
> >>> +
> >>> +next:
> >>> + sock_put(sk);
> >>> + cond_resched();
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
> >>> {
> >>> struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
> >>> struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
> >>> struct mptcp_pm_addr_entry addr, *entry;
> >>> + struct net *net = sock_net(skb->sk);
> >>> int ret;
> >>>
> >>> ret = mptcp_pm_parse_addr(attr, info, true, &addr);
> >>> if (ret < 0)
> >>> return ret;
> >>>
> >>> + if (addr.addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP) {
> >>> + list_for_each_entry(entry, &pernet->local_addr_list, list) {
> >>> + if (addresses_equal(&entry->addr, &addr.addr, false))
> >>> + mptcp_nl_addr_backup(net, &entry->addr);
> >>
> >> I think a more strict match should be required here that includes the
> >> 'id' as well. There may be multiple endpoints with the same address but
> >> different port numbers.
> >>
> >
> > Adding 'id' match doesn't work, since addr.addr's id is always 0 here.
> >
> > I think we can use addresses_equal(&entry->addr, &addr.addr, true) here,
> > it's enough to cover the same address with different port numbers case.
>
> Ok, yes, addresses_equal() does compare port numbers so that should help.
>
> >>> + }
> >>> + return 0;
> >>> + }
> >>> +
> >>> entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> >>> if (!entry) {
> >>> GENL_SET_ERR_MSG(info, "can't allocate addr");
> >>> --
> >>> 2.26.2
>
> --
> Mat Martineau
> Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [MPTCP] Re: [MPTCP][PATCH mptcp-next 3/5] mptcp: deal with MPTCP_PM_ADDR_FLAG_BACKUP in PM netlink
@ 2020-12-04 10:54 Paolo Abeni
0 siblings, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2020-12-04 10:54 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 5204 bytes --]
On Wed, 2020-12-02 at 12:35 +0800, Geliang Tang wrote:
> Hi Mat,
>
> Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年12月2日周三 上午8:40写道：
> > On Mon, 30 Nov 2020, Geliang Tang wrote:
> >
> > > Hi Mat,
> > >
> > > Thanks for your review.
> > >
> > > Mat Martineau <mathew.j.martineau(a)linux.intel.com> 于2020年11月21日周六 上午9:17写道：
> > > > On Fri, 20 Nov 2020, Geliang Tang wrote:
> > > >
> > > > > This patch added the MP_PRIO support for PM netlink:
> > > > >
> > > > > When PM netlink adds an address, check whether the
> > > > > MPTCP_PM_ADDR_FLAG_BACKUP flag is set. If it is, check whether this
> > > > > address had been added in the local address list. If it had been, then
> > > > > call mptcp_nl_addr_backup to deal with this address.
> > > > >
> > > > > In mptcp_nl_addr_backup, we traverse all the existing msk sockets to find
> > > > > the relevant sockets, and get the backup value according to whether the
> > > > > MPTCP_PM_ADDR_FLAG_BACKUP flag is set, then call
> > > > > mptcp_pm_nl_mp_prio_send_ack to send out a MP_PRIO ACK packet.
> > > > >
> > > > > In mptcp_pm_nl_mp_prio_send_ack, we set or clear the
> > > > > MPTCP_PM_ADDR_FLAG_BACKUP flag.
> > > > >
> > > > > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > > > > ---
> > > > > net/mptcp/pm_netlink.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 41 insertions(+)
> > > > >
> > > > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > > > index dd48af3c5ed5..58960e1cdc32 100644
> > > > > --- a/net/mptcp/pm_netlink.c
> > > > > +++ b/net/mptcp/pm_netlink.c
> > > > > @@ -535,6 +535,11 @@ void mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> > > > > return;
> > > > > }
> > > > >
> > > > > + if (addr->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> > > > > + addr->flags &= ‾MPTCP_PM_ADDR_FLAG_BACKUP;
> > > > > + else
> > > > > + addr->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
> > > > > +
> > > >
> > > > Why is the backup bit inverted here? It's not clear why it's changed, and
> > > > why bkup is passed in based on the value of the bit when read by the
> > > > calling function.
> > > >
> > >
> > > I think we can use backup command not only to 'turn on' a subflow's backup
> > > bit, but also to 'turn off' a subflow's backup bit. So the pm_nl_ctl backup
> > > command will work like this:
> > >
> > > # pm_nl_ctl add 10.0.2.1 flags signal; pm_nl_ctl dump
> > > id 1 flags signal 10.0.2.1
> > > # pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
> > > id 1 flags signal,backup 10.0.2.1
> > > # pm_nl_ctl add 10.0.2.1 flags backup; pm_nl_ctl dump
> > > id 1 flags signal 10.0.2.1
> >
> > I would prefer to have the 'backup' bit in the netlink flags set the
> > backup bit directly - if the backup flag is 0 the subflow backup bit gets
> > set to 0, and if the netlink backup flag is 1 the subflow backup bit gets
> > set to 1. Paolo, does that interpretation of the backup flag match what
> > you were thinking too?
>
> Should we add an argument behind backup for the pm_nl_ctl tool? Then we
> can use pm_nl_ctl to set backup flag like this:
>
> pm_nl_ctl add 10.0.2.1 flags backup 1
> pm_nl_ctl add 10.0.2.1 flags backup 0
>
> Or add a new argument 'set' instead of using 'add' when changing
> the backup flag:
>
> pm_nl_ctl set 10.0.2.1 flags backup 1
> pm_nl_ctl set 10.0.2.1 flags backup 0
This patch changes the semantic of the MPTCP_PM_CMD_ADD_ADDR netlink
command depending on the presence of the backup flag.
I think is quite confusing.
The backup flag should be the only attribute we are allowed to change
on an existing endpoint, but that may change in the future - e.g. if we
will ever allow setting setsockopt() on the listening socket for port-
based endpoint.
I think it would be better add another NL command to update the
existing address. It may still use the same parsing helper
as MPTCP_PM_CMD_ADD_ADDR, than it will lookup the address by ID and
will set the address backup flag according to the value provided by the
user.
The pm_nl_ctl syntax is not extremely important IMHO, but the ip route
syntax will be much more interesting.
Something alike:
ip mptcp endpoint set id <nr> flags backup # the endpoint become a backup one
ip mptcp endpoint set id <nr> flags nobackup # the endpoint become a non backup one
ip mptcp endpoint set id <nr> flags "" # the endpoint become a non backup one
ip mptcp endpoint set id <nr> flags signal # the endpoint become a non backup one, 'signal' is ignored
ip mptcp endpoint set id <nr> flags signal,backup # the endpoint become a backup one, 'signal' is ignored
pm_nl_ctl could opt for something similar, e.g.:
pm_nl_ctl set 10.0.2.1 flags backup
pm_nl_ctl set 10.0.2.1 flags nobackup
pm_nl_ctl set 10.0.2.1 flags signal,backup
pm_nl_ctl set 10.0.2.1 flags signal
etc...
The only special thing will ne the 'nobackup' keyword, just to be more
expressive than leaving the flag list empty.
WDYT?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-04 10:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-21 1:17 [MPTCP] Re: [MPTCP][PATCH mptcp-next 3/5] mptcp: deal with MPTCP_PM_ADDR_FLAG_BACKUP in PM netlink Mat Martineau
-- strict thread matches above, loose matches on Subject: below --
2020-11-30 2:37 Geliang Tang
2020-12-02 0:40 Mat Martineau
2020-12-02 4:35 Geliang Tang
2020-12-04 10:54 Paolo Abeni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox