[PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
to the pnetid table and will attach the <pnetid> to net device
whose name is <ethx>. But When do SMCR by <ethx>, in function
smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
use of the pnetid seems weird. Sometimes it is difficult to know
the hierarchy of net device what may make it difficult to configure
the pnetid and to use the pnetid. Looking into the history of
commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
that changes the ndev from the <ethx> to the <ethx>'s base ndev
when finding pnetid by pnetid table. It seems a mistake.

This patch changes the ndev back to the <ethx> when finding pnetid
by pnetid table.

Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
---
 net/smc/smc_pnet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 716808f374a8..cc098780970b 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -1079,14 +1079,15 @@ static void smc_pnet_find_roce_by_pnetid(struct net_device *ndev,
 					 struct smc_init_info *ini)
 {
 	u8 ndev_pnetid[SMC_MAX_PNETID_LEN];
+	struct net_device *base_ndev;
 	struct net *net;
 
-	ndev = pnet_find_base_ndev(ndev);
+	base_ndev = pnet_find_base_ndev(ndev);
 	net = dev_net(ndev);
-	if (smc_pnetid_by_dev_port(ndev->dev.parent, ndev->dev_port,
+	if (smc_pnetid_by_dev_port(base_ndev->dev.parent, base_ndev->dev_port,
 				   ndev_pnetid) &&
 	    smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid)) {
-		smc_pnet_find_rdma_dev(ndev, ini);
+		smc_pnet_find_rdma_dev(base_ndev, ini);
 		return; /* pnetid could not be determined */
 	}
 	_smc_pnet_find_roce_by_pnetid(ndev_pnetid, ini, NULL, net);
-- 
2.24.3 (Apple Git-128)

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Jakub Kicinski]

On Fri, 27 Dec 2024 12:04:55 +0800 Guangguan Wang wrote:
> The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
> to the pnetid table and will attach the <pnetid> to net device
> whose name is <ethx>. But When do SMCR by <ethx>, in function
> smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
> pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
> use of the pnetid seems weird. Sometimes it is difficult to know
> the hierarchy of net device what may make it difficult to configure
> the pnetid and to use the pnetid. Looking into the history of
> commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
> that changes the ndev from the <ethx> to the <ethx>'s base ndev
> when finding pnetid by pnetid table. It seems a mistake.
> 
> This patch changes the ndev back to the <ethx> when finding pnetid
> by pnetid table.

SMC maintainers, please review..

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Wen Gu]

On 2024/12/27 12:04, Guangguan Wang wrote:
> The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
> to the pnetid table and will attach the <pnetid> to net device
> whose name is <ethx>. But When do SMCR by <ethx>, in function
> smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
> pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
> use of the pnetid seems weird. Sometimes it is difficult to know
> the hierarchy of net device what may make it difficult to configure
> the pnetid and to use the pnetid. Looking into the history of
> commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
> that changes the ndev from the <ethx> to the <ethx>'s base ndev
> when finding pnetid by pnetid table. It seems a mistake.
> 
> This patch changes the ndev back to the <ethx> when finding pnetid
> by pnetid table.
> 
> Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>

It makes sense to me, thanks!

Reviewed-by: Wen Gu <guwen@linux.alibaba.com>

> ---
>   net/smc/smc_pnet.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
> index 716808f374a8..cc098780970b 100644
> --- a/net/smc/smc_pnet.c
> +++ b/net/smc/smc_pnet.c
> @@ -1079,14 +1079,15 @@ static void smc_pnet_find_roce_by_pnetid(struct net_device *ndev,
>   					 struct smc_init_info *ini)
>   {
>   	u8 ndev_pnetid[SMC_MAX_PNETID_LEN];
> +	struct net_device *base_ndev;
>   	struct net *net;
>   
> -	ndev = pnet_find_base_ndev(ndev);
> +	base_ndev = pnet_find_base_ndev(ndev);
>   	net = dev_net(ndev);
> -	if (smc_pnetid_by_dev_port(ndev->dev.parent, ndev->dev_port,
> +	if (smc_pnetid_by_dev_port(base_ndev->dev.parent, base_ndev->dev_port,
>   				   ndev_pnetid) &&
>   	    smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid)) {
> -		smc_pnet_find_rdma_dev(ndev, ini);
> +		smc_pnet_find_rdma_dev(base_ndev, ini);
>   		return; /* pnetid could not be determined */
>   	}
>   	_smc_pnet_find_roce_by_pnetid(ndev_pnetid, ini, NULL, net);

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Paolo Abeni]

On 12/27/24 5:04 AM, Guangguan Wang wrote:
> The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
> to the pnetid table and will attach the <pnetid> to net device
> whose name is <ethx>. But When do SMCR by <ethx>, in function
> smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
> pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
> use of the pnetid seems weird. Sometimes it is difficult to know
> the hierarchy of net device what may make it difficult to configure
> the pnetid and to use the pnetid. Looking into the history of
> commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
> that changes the ndev from the <ethx> to the <ethx>'s base ndev
> when finding pnetid by pnetid table. It seems a mistake.
> 
> This patch changes the ndev back to the <ethx> when finding pnetid
> by pnetid table.
> 
> Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>

If I read correctly, this will break existing applications using the
lookup schema introduced by the blamed commit - which is not very recent.

Perhaps for a net patch would be better to support both lookup schemas i.e.

	(smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid) ||
	 smc_pnet_find_ndev_pnetid_by_table(base_ndev, ndev_pnetid))

?

Thanks,

Paolo

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Tue, 7 Jan 2025 09:44:30 +0100
Paolo Abeni <pabeni@redhat.com> wrote:

> On 12/27/24 5:04 AM, Guangguan Wang wrote:

@Guangguan Wang: please use my linux.ibm.com address
in the future.

> > The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
> > to the pnetid table and will attach the <pnetid> to net device
> > whose name is <ethx>. But When do SMCR by <ethx>, in function
> > smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
> > pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
> > use of the pnetid seems weird. Sometimes it is difficult to know
> > the hierarchy of net device what may make it difficult to configure
> > the pnetid and to use the pnetid. Looking into the history of
> > commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
> > that changes the ndev from the <ethx> to the <ethx>'s base ndev
> > when finding pnetid by pnetid table. It seems a mistake.
> > 
> > This patch changes the ndev back to the <ethx> when finding pnetid
> > by pnetid table.
> > 
> > Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
> > Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>  
> 
> If I read correctly, this will break existing applications using the
> lookup schema introduced by the blamed commit - which is not very
> recent.

Hi Paolo,

sorry for chiming in late. Wenjia is on vacation and Jan is out sick!
After some reading and thinking I could not figure out how 890a2cb4a966
("net/smc: rework pnet table") is broken.

Admittedly I'm not really a net guy,and I'm mostly guessing what that
lower and upper device stuff is, so please bear with me. All that said, I
do think that going to the lowest netdev in the hierarchy is a sane
thing to do here.  I assume  that lower and upper devices are applicable
to stuff like bonding. 

PNETID stands for "Physical Network Identifier" and the idea is that iff
two ports are connected to the same physical network then they should
have the same PNETID. And on s390 PNETID can come and often is comming
"from the hardware". Now for something like a bond of two OSA
interfaces, I would expect the two legs of the bond to probably have a
"HW PNETID", but the netdev representing the bond itself won't have one
unless the Linux admin defines a software PNETID, which is work, and
can't have a HW PNETID because it is a software construct within Linux.
Breaking for example an active-backup bond setup where the legs have
HW PNETIDs and the admin did not bother to specify a PNETID for the bond
is not acceptable.

Let me also note that if ndev is a leaf (i.e. there is no lower device to
it) then ndev == base_ndev, and the whole discussion does not matter for
that case.

Again I have to emphasize that my domain knowledge is very limited, but
I really don't feel comfortable going forward with this without Jan or
Wenjia weighing in on the matter.

Paolo thanks for bringing this up!

> 
> Perhaps for a net patch would be better to support both lookup schemas
> i.e.
> 
> 	(smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid) ||
> 	 smc_pnet_find_ndev_pnetid_by_table(base_ndev, ndev_pnetid))
> 
> ?
>

Hm, I guess the idea here is that if ndev has a PNETID then it should
take precedence, but if not we should try to obtain the PNETID of its
"base_ndev". I'm not sure this would make things better compared to the
original idea of caring about the leaf. Which makes me question my
understanding of the problem statement from the commit message.

BTW to implement the logic proposed by you Paolo, as understood by me,
we would have to use "&&" instead of "||". The whole expression is
supposed
to evaluate to false if a pnetid is found and to true if no pnet_id is
found. smc_pnet_find_ndev_pnetid_by_table(ndev) returns false if a pnetid
is found. I.e. if not found we would just short circuit to true and not
call smc_pnet_find_ndev_pnetid_by_table(base_ndev), which is not what I
believe you wanted to propose.

To sum it up, please let us wait until Wenjia or Jan chime in. Copying
Alexandra as well: she is more of a net person than I am, and maybe she
has a more informed opinion.

Regards,
Halil
[...]

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/1/8 03:32, Halil Pasic wrote:
> On Tue, 7 Jan 2025 09:44:30 +0100
> Paolo Abeni <pabeni@redhat.com> wrote:
> 
>> On 12/27/24 5:04 AM, Guangguan Wang wrote:
> 
> @Guangguan Wang: please use my linux.ibm.com address
> in the future.
Get it.

> 
>>> The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
>>> to the pnetid table and will attach the <pnetid> to net device
>>> whose name is <ethx>. But When do SMCR by <ethx>, in function
>>> smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
>>> pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
>>> use of the pnetid seems weird. Sometimes it is difficult to know
>>> the hierarchy of net device what may make it difficult to configure
>>> the pnetid and to use the pnetid. Looking into the history of
>>> commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
>>> that changes the ndev from the <ethx> to the <ethx>'s base ndev
>>> when finding pnetid by pnetid table. It seems a mistake.
>>>
>>> This patch changes the ndev back to the <ethx> when finding pnetid
>>> by pnetid table.
>>>
>>> Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
>>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>  
>>
>> If I read correctly, this will break existing applications using the
>> lookup schema introduced by the blamed commit - which is not very
>> recent.
> 
> Hi Paolo,
> 
> sorry for chiming in late. Wenjia is on vacation and Jan is out sick!
> After some reading and thinking I could not figure out how 890a2cb4a966
> ("net/smc: rework pnet table") is broken.

Before commit 890a2cb4a966:
smc_pnet_find_roce_resource
    smc_pnet_find_roce_by_pnetid(ndev, ...) /* lookup via hardware-defined pnetid */
        smc_pnetid_by_dev_port(base_ndev, ...)
    smc_pnet_find_roce_by_table(ndev, ...) /* lookup via SMC PNET table */
    {
        ...
        list_for_each_entry(pnetelem, &smc_pnettable.pnetlist, list) {
                if (ndev == pnetelem->ndev) { /* notice here, it was ndev to matching pnetid element in pnet table */
        ...
    }

After commit 890a2cb4a966:
smc_pnet_find_roce_resource
    smc_pnet_find_roce_by_pnetid
    {
        ...
        base_ndev = pnet_find_base_ndev(ndev); /* rename the variable name to base_ndev for better understanding */
        smc_pnetid_by_dev_port(base_ndev, ...)
        smc_pnet_find_ndev_pnetid_by_table(base_ndev, ...)
        {
                ...
                list_for_each_entry(pnetelem, &smc_pnettable.pnetlist, list) {
                if (base_ndev == pnetelem->ndev) { /* notice here, it is base_ndev to matching pnetid element in pnet table */
                ...
        }

    }

The commit 890a2cb4a966 has changed ndev to base_ndev when matching pnetid element in pnet table.
But in the function smc_pnet_add_eth, the pnetid is attached to the ndev itself, not the base_ndev.
smc_pnet_add_eth(...)
{
    ...
    ndev = dev_get_by_name(net, eth_name);
    ...
        if (new_netdev) {
            if (ndev) {
                new_pe->ndev = ndev;
                netdev_tracker_alloc(ndev, &new_pe->dev_tracker,
                    GFP_ATOMIC);
            }
            list_add_tail(&new_pe->list, &pnettable->pnetlist);
            mutex_unlock(&pnettable->lock);
        } else {
    ...
}

> 
> Admittedly I'm not really a net guy,and I'm mostly guessing what that
> lower and upper device stuff is, so please bear with me. All that said, I
> do think that going to the lowest netdev in the hierarchy is a sane
> thing to do here.  I assume  that lower and upper devices are applicable
> to stuff like bonding. 
> 
> PNETID stands for "Physical Network Identifier" and the idea is that iff
> two ports are connected to the same physical network then they should
> have the same PNETID. And on s390 PNETID can come and often is comming
> "from the hardware". Now for something like a bond of two OSA
> interfaces, I would expect the two legs of the bond to probably have a
> "HW PNETID", but the netdev representing the bond itself won't have one
> unless the Linux admin defines a software PNETID, which is work, and
> can't have a HW PNETID because it is a software construct within Linux.
> Breaking for example an active-backup bond setup where the legs have
> HW PNETIDs and the admin did not bother to specify a PNETID for the bond
> is not acceptable.
> 
> Let me also note that if ndev is a leaf (i.e. there is no lower device to
> it) then ndev == base_ndev, and the whole discussion does not matter for
> that case.
> 
> Again I have to emphasize that my domain knowledge is very limited, but
> I really don't feel comfortable going forward with this without Jan or
> Wenjia weighing in on the matter.
> 
> Paolo thanks for bringing this up!
> 
>>
>> Perhaps for a net patch would be better to support both lookup schemas
>> i.e.
>>
>> 	(smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid) ||
>> 	 smc_pnet_find_ndev_pnetid_by_table(base_ndev, ndev_pnetid))
>>
>> ?
>>
> 
> Hm, I guess the idea here is that if ndev has a PNETID then it should
> take precedence, but if not we should try to obtain the PNETID of its
> "base_ndev". I'm not sure this would make things better compared to the
> original idea of caring about the leaf. Which makes me question my
> understanding of the problem statement from the commit message.
> 
> BTW to implement the logic proposed by you Paolo, as understood by me,
> we would have to use "&&" instead of "||". The whole expression is
> supposed
> to evaluate to false if a pnetid is found and to true if no pnet_id is
> found. smc_pnet_find_ndev_pnetid_by_table(ndev) returns false if a pnetid
> is found. I.e. if not found we would just short circuit to true and not
> call smc_pnet_find_ndev_pnetid_by_table(base_ndev), which is not what I
> believe you wanted to propose.

Yes, it should be
        (smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid) &&
 	 smc_pnet_find_ndev_pnetid_by_table(base_ndev, ndev_pnetid))
if for the consideration of application's compatible usage of smc_pnet.

Thanks,
Guangguan Wang

> 
> To sum it up, please let us wait until Wenjia or Jan chime in. Copying
> Alexandra as well: she is more of a net person than I am, and maybe she
> has a more informed opinion.
> 
> Regards,
> Halil
> [...]
> 

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Alexandra Winter]

On 07.01.25 20:32, Halil Pasic wrote:
> On Tue, 7 Jan 2025 09:44:30 +0100
> Paolo Abeni <pabeni@redhat.com> wrote:
> 
>> On 12/27/24 5:04 AM, Guangguan Wang wrote:
...
>>> The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
>>> to the pnetid table and will attach the <pnetid> to net device
>>> whose name is <ethx>. But When do SMCR by <ethx>, in function
>>> smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
>>> pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
>>> use of the pnetid seems weird. Sometimes it is difficult to know
>>> the hierarchy of net device what may make it difficult to configure
>>> the pnetid and to use the pnetid. Looking into the history of
>>> commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
>>> that changes the ndev from the <ethx> to the <ethx>'s base ndev
>>> when finding pnetid by pnetid table. It seems a mistake.
>>>
>>> This patch changes the ndev back to the <ethx> when finding pnetid
>>> by pnetid table.
>>>
>>> Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
>>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>  
>>
>> If I read correctly, this will break existing applications using the
>> lookup schema introduced by the blamed commit - which is not very
>> recent.


I agree that this patch may break existing applications or existing
configuration automation scripts.

...
> PNETID stands for "Physical Network Identifier" and the idea is that iff
> two ports are connected to the same physical network then they should
> have the same PNETID. And on s390 PNETID can come and often is comming
> "from the hardware". 
...


HW pnetids (smc_pnetid_by_dev_port()) are only visible at the base netdevice.
It seems that the pnetid table, managed by the smc_pnet tool, tries to mimick
that behaviour, and the concept (recommendation?) would be to set the 
user-defined pnetid also for the base netdevice and then use the upper
level netdevices for the tcp connection. Which makes some sense, 
all upper level devices have the same connectivity as the base device.

So this patch would break a setup that follows this concept and only sets the 
pnetid at the base netdevice.


Optionally you can set a user-defined pnetid on upper level devices (maybe for
usability??), but as Guangguan noticed, that has no practical impact.
In the documentation I see examples where the same pnetid is set for upper
and base device. 
You cannot set a user-defined pnetid on a upper level device, if the base
device has a HW pnetid (smc_pnet_add_eth()) which makes some sense,
not even the same pnetid (makes less sense IMO).
However you can set different user-defined pnetids on the upper netdevices
and the base device, which makes no sense to me.

>>
>> Perhaps for a net patch would be better to support both lookup schemas
>> i.e.
>>
>> 	(smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid) ||
>> 	 smc_pnet_find_ndev_pnetid_by_table(base_ndev, ndev_pnetid))
>>
>> ?

This may yield undesirable results, if base pnetid and upper pnetid differ..
But then maybe GIGO?


... 
> BTW to implement the logic proposed by you Paolo, as understood by me,
> we would have to use "&&" instead of "||". 
+1


Another idea may be to change the setting of a user-defined pnetid
on an upper level netdevice to
- fail if the base netdevice has a different pnetid
- set the pnetid of the base device , if it is currently unset.

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Wed, 8 Jan 2025 12:57:00 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> > sorry for chiming in late. Wenjia is on vacation and Jan is out sick!
> > After some reading and thinking I could not figure out how 890a2cb4a966
> > ("net/smc: rework pnet table") is broken.  
> 
> Before commit 890a2cb4a966:
> smc_pnet_find_roce_resource
>     smc_pnet_find_roce_by_pnetid(ndev, ...) /* lookup via hardware-defined pnetid */
>         smc_pnetid_by_dev_port(base_ndev, ...)
>     smc_pnet_find_roce_by_table(ndev, ...) /* lookup via SMC PNET table */
>     {
>         ...
>         list_for_each_entry(pnetelem, &smc_pnettable.pnetlist, list) {
>                 if (ndev == pnetelem->ndev) { /* notice here, it was ndev to matching pnetid element in pnet table */
>         ...
>     }
> 
> After commit 890a2cb4a966:
> smc_pnet_find_roce_resource
>     smc_pnet_find_roce_by_pnetid
>     {
>         ...
>         base_ndev = pnet_find_base_ndev(ndev); /* rename the variable name to base_ndev for better understanding */
>         smc_pnetid_by_dev_port(base_ndev, ...)
>         smc_pnet_find_ndev_pnetid_by_table(base_ndev, ...)
>         {
>                 ...
>                 list_for_each_entry(pnetelem, &smc_pnettable.pnetlist, list) {
>                 if (base_ndev == pnetelem->ndev) { /* notice here, it is base_ndev to matching pnetid element in pnet table */
>                 ...
>         }
> 
>     }
> 
> The commit 890a2cb4a966 has changed ndev to base_ndev when matching pnetid element in pnet table.
> But in the function smc_pnet_add_eth, the pnetid is attached to the ndev itself, not the base_ndev.
> smc_pnet_add_eth(...)
> {
>     ...
>     ndev = dev_get_by_name(net, eth_name);
>     ...
>         if (new_netdev) {
>             if (ndev) {
>                 new_pe->ndev = ndev;
>                 netdev_tracker_alloc(ndev, &new_pe->dev_tracker,
>                     GFP_ATOMIC);
>             }
>             list_add_tail(&new_pe->list, &pnettable->pnetlist);
>             mutex_unlock(&pnettable->lock);
>         } else {
>     ...
> }

I still not understand why do you think that 890a2cb4a966~1 is better
than 890a2cb4a966 even if things changed with 890a2cb4a966 which
I did not verify for myself but am willing to assume.

Is there some particular setup that you think would benefit from
you patch? I.e. going back to the 890a2cb4a966~1 behavior I suppose.

I think I showed a valid and practical setup that would break with your
patch as is. Do you agree with that statement?

Regards,
Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/1/9 11:04, Halil Pasic wrote:
> On Wed, 8 Jan 2025 12:57:00 +0800
> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
> 
>>> sorry for chiming in late. Wenjia is on vacation and Jan is out sick!
>>> After some reading and thinking I could not figure out how 890a2cb4a966
>>> ("net/smc: rework pnet table") is broken.  
>>
>> Before commit 890a2cb4a966:
>> smc_pnet_find_roce_resource
>>     smc_pnet_find_roce_by_pnetid(ndev, ...) /* lookup via hardware-defined pnetid */
>>         smc_pnetid_by_dev_port(base_ndev, ...)
>>     smc_pnet_find_roce_by_table(ndev, ...) /* lookup via SMC PNET table */
>>     {
>>         ...
>>         list_for_each_entry(pnetelem, &smc_pnettable.pnetlist, list) {
>>                 if (ndev == pnetelem->ndev) { /* notice here, it was ndev to matching pnetid element in pnet table */
>>         ...
>>     }
>>
>> After commit 890a2cb4a966:
>> smc_pnet_find_roce_resource
>>     smc_pnet_find_roce_by_pnetid
>>     {
>>         ...
>>         base_ndev = pnet_find_base_ndev(ndev); /* rename the variable name to base_ndev for better understanding */
>>         smc_pnetid_by_dev_port(base_ndev, ...)
>>         smc_pnet_find_ndev_pnetid_by_table(base_ndev, ...)
>>         {
>>                 ...
>>                 list_for_each_entry(pnetelem, &smc_pnettable.pnetlist, list) {
>>                 if (base_ndev == pnetelem->ndev) { /* notice here, it is base_ndev to matching pnetid element in pnet table */
>>                 ...
>>         }
>>
>>     }
>>
>> The commit 890a2cb4a966 has changed ndev to base_ndev when matching pnetid element in pnet table.
>> But in the function smc_pnet_add_eth, the pnetid is attached to the ndev itself, not the base_ndev.
>> smc_pnet_add_eth(...)
>> {
>>     ...
>>     ndev = dev_get_by_name(net, eth_name);
>>     ...
>>         if (new_netdev) {
>>             if (ndev) {
>>                 new_pe->ndev = ndev;
>>                 netdev_tracker_alloc(ndev, &new_pe->dev_tracker,
>>                     GFP_ATOMIC);
>>             }
>>             list_add_tail(&new_pe->list, &pnettable->pnetlist);
>>             mutex_unlock(&pnettable->lock);
>>         } else {
>>     ...
>> }
> 
> I still not understand why do you think that 890a2cb4a966~1 is better
> than 890a2cb4a966 even if things changed with 890a2cb4a966 which
> I did not verify for myself but am willing to assume.
> 
> Is there some particular setup that you think would benefit from
> you patch? I.e. going back to the 890a2cb4a966~1 behavior I suppose.
> 

We want to use SMC in container on cloud environment, and encounter problem
when using smc_pnet with commit 890a2cb4a966. In container, there have choices
of different container network, such as directly using host network, virtual
network IPVLAN, veth, etc. Different choices of container network have different
netdev hierarchy. Examples of netdev hierarchy show below. (eth0 and eth1 in host
below is the netdev directly related to the physical device).
 _______________________________      ________________________________   
|   _________________           |     |   _________________           |  
|  |POD              |          |     |  |POD  __________  |          |  
|  |                 |          |     |  |    |upper_ndev| |          |  
|  | eth0_________   |          |     |  |eth0|__________| |          |  
|  |____|         |__|          |     |  |_______|_________|          |  
|       |         |             |     |          |lower netdev        |  
|       |         |             |     |        __|______              |  
|   eth1|base_ndev| eth0_______ |     |   eth1|         | eth0_______ |  
|       |         |    | RDMA  ||     |       |base_ndev|    | RDMA  ||  
| host  |_________|    |_______||     | host  |_________|    |_______||  
———————————————————————————————-      ———————————————————————————————-    
 netdev hierarchy if directly          netdev hierarchy if using IPVLAN    
   using host network
 _______________________________
|   _____________________       |
|  |POD        _________ |      |
|  |          |base_ndev||      |
|  |eth0(veth)|_________||      |
|  |____________|________|      |
|               |pairs          |
|        _______|_              |
|       |         | eth0_______ |
|   veth|base_ndev|    | RDMA  ||
|       |_________|    |_______||
|        _________              |
|   eth1|base_ndev|             |
| host  |_________|             |
 ———————————————————————————————
  netdev hierarchy if using veth

Due to some reasons, the eth1 in host is not RDMA attached netdevice, pnetid
is needed to map the eth1(in host) with RDMA device so that POD can do SMC-R.
Because the eth1(in host) is managed by CNI plugin(such as Terway, network
management plugin in container environment), and in cloud environment the
eth(in host) can dynamically be inserted by CNI when POD create and dynamically
be removed by CNI when POD destroy and no POD related to the eth(in host)
anymore. It is hard for us to config the pnetid to the eth1(in host). So we
config the pnetid to the netdevice which can be seen in POD. When do SMC-R, both
the container directly using host network and the container using veth network
can successfully match the RDMA device, because the configured pnetid netdev is a
base_ndev. But the container using IPVLAN can not successfully match the RDMA
device and 0x03030000 fallback happens, because the configured pnetid netdev is
not a base_ndev. Additionally, if config pnetid to the eth1(in host) also can not
work for matching RDMA device when using veth network and doing SMC-R in POD.

My patch can resolve the problem we encountered and also can unify the pnetid setup
of different network choices list above, assuming the pnetid is not limited to
config to the base_ndev directly related to the physical device(indeed, the current
implementation has not limited it yet).

> I think I showed a valid and practical setup that would break with your
> patch as is. Do you agree with that statement?
Did you mean
"
Now for something like a bond of two OSA
interfaces, I would expect the two legs of the bond to probably have a
"HW PNETID", but the netdev representing the bond itself won't have one
unless the Linux admin defines a software PNETID, which is work, and
can't have a HW PNETID because it is a software construct within Linux.
Breaking for example an active-backup bond setup where the legs have
HW PNETIDs and the admin did not bother to specify a PNETID for the bond
is not acceptable.
" ?
If the legs have HW pnetids, add pnetid to bond netdev will fail as
smc_pnet_add_eth will check whether the base_ndev already have HW pnetid.

If the legs without HW pnetids, and admin add pnetids to legs through smc_pnet.
Yes, my patch will break the setup. What Paolo suggests(both checking ndev and
base_ndev, and replace || by && )can help compatible with the setup.


Thanks,
Guangguan Wang
> 
> Regards,
> Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/1/9 00:00, Alexandra Winter wrote:
> 
> 
> On 07.01.25 20:32, Halil Pasic wrote:
>> On Tue, 7 Jan 2025 09:44:30 +0100
>> Paolo Abeni <pabeni@redhat.com> wrote:
>>
>>> On 12/27/24 5:04 AM, Guangguan Wang wrote:
> ...
>>>> The command 'smc_pnet -a -I <ethx> <pnetid>' will add <pnetid>
>>>> to the pnetid table and will attach the <pnetid> to net device
>>>> whose name is <ethx>. But When do SMCR by <ethx>, in function
>>>> smc_pnet_find_roce_by_pnetid, it will use <ethx>'s base ndev's
>>>> pnetid to match rdma device, not <ethx>'s pnetid. The asymmetric
>>>> use of the pnetid seems weird. Sometimes it is difficult to know
>>>> the hierarchy of net device what may make it difficult to configure
>>>> the pnetid and to use the pnetid. Looking into the history of
>>>> commit, it was the commit 890a2cb4a966 ("net/smc: rework pnet table")
>>>> that changes the ndev from the <ethx> to the <ethx>'s base ndev
>>>> when finding pnetid by pnetid table. It seems a mistake.
>>>>
>>>> This patch changes the ndev back to the <ethx> when finding pnetid
>>>> by pnetid table.
>>>>
>>>> Fixes: 890a2cb4a966 ("net/smc: rework pnet table")
>>>> Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>  
>>>
>>> If I read correctly, this will break existing applications using the
>>> lookup schema introduced by the blamed commit - which is not very
>>> recent.
> 
> 
> I agree that this patch may break existing applications or existing
> configuration automation scripts.
> 
> ...
>> PNETID stands for "Physical Network Identifier" and the idea is that iff
>> two ports are connected to the same physical network then they should
>> have the same PNETID. And on s390 PNETID can come and often is comming
>> "from the hardware". 
> ...
> 
> 
> HW pnetids (smc_pnetid_by_dev_port()) are only visible at the base netdevice.
> It seems that the pnetid table, managed by the smc_pnet tool, tries to mimick
> that behaviour, and the concept (recommendation?) would be to set the 
> user-defined pnetid also for the base netdevice and then use the upper
> level netdevices for the tcp connection. Which makes some sense, 
> all upper level devices have the same connectivity as the base device.
> 
> So this patch would break a setup that follows this concept and only sets the 
> pnetid at the base netdevice.
>
Hi Alexandra,

See the examples of using smc-r in container on cloud environment here
https://lore.kernel.org/linux-s390/3ff078e0-150d-41ba-b705-a8e0365f0370@linux.ibm.com/T/#t.

Especially the example of using veth in container, it can not successfully match the expected
RDMA device when do SMC-R in POD, if follow the concept of the HW pnetid and only sets the
pnetid at the base netdevice.

Maybe it is time to extend the concept and usage of pnetid table?

> 
> Optionally you can set a user-defined pnetid on upper level devices (maybe for
> usability??), but as Guangguan noticed, that has no practical impact.
> In the documentation I see examples where the same pnetid is set for upper
> and base device. 
> You cannot set a user-defined pnetid on a upper level device, if the base
> device has a HW pnetid (smc_pnet_add_eth()) which makes some sense,
> not even the same pnetid (makes less sense IMO).

> However you can set different user-defined pnetids on the upper netdevices
> and the base device, which makes no sense to me.
> 
>>>
>>> Perhaps for a net patch would be better to support both lookup schemas
>>> i.e.
>>>
>>> 	(smc_pnet_find_ndev_pnetid_by_table(ndev, ndev_pnetid) ||
>>> 	 smc_pnet_find_ndev_pnetid_by_table(base_ndev, ndev_pnetid))
>>>
>>> ?
> 
> This may yield undesirable results, if base pnetid and upper pnetid differ..
> But then maybe GIGO?
> 
> 
> ... 
>> BTW to implement the logic proposed by you Paolo, as understood by me,
>> we would have to use "&&" instead of "||". 
> +1
> 
> 
> Another idea may be to change the setting of a user-defined pnetid
> on an upper level netdevice to
> - fail if the base netdevice has a different pnetid
> - set the pnetid of the base device , if it is currently unset.
> 
In the example of veth, the base ndev found through the upper level ndev in POD
is not the real "base device" describe here.
And I wonder whether it is confused if set a user-defined pnetid to one netdevice,
but list another netdevice when smc_pnet show. For example set pnetid ABC to eth1,
whose base netdev is eth0, but when smc_pnet show, only can find eth0 with pnetid
ABC.

Thanks,
Guangguan Wang

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Fri, 10 Jan 2025 13:43:44 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> > I think I showed a valid and practical setup that would break with your
> > patch as is. Do you agree with that statement?  
> Did you mean
> "
> Now for something like a bond of two OSA
> interfaces, I would expect the two legs of the bond to probably have a
> "HW PNETID", but the netdev representing the bond itself won't have one
> unless the Linux admin defines a software PNETID, which is work, and
> can't have a HW PNETID because it is a software construct within Linux.
> Breaking for example an active-backup bond setup where the legs have
> HW PNETIDs and the admin did not bother to specify a PNETID for the bond
> is not acceptable.
> " ?
> If the legs have HW pnetids, add pnetid to bond netdev will fail as
> smc_pnet_add_eth will check whether the base_ndev already have HW pnetid.
> 
> If the legs without HW pnetids, and admin add pnetids to legs through smc_pnet.
> Yes, my patch will break the setup. What Paolo suggests(both checking ndev and
> base_ndev, and replace || by && )can help compatible with the setup.

I'm glad we agree on that part. Things are much more acceptable if we
are doing both base and ndev. Nevertheless I would like to understand
your problem better, and talk about it to my team. I will also ask some
questions in another email.

That said having things work differently if there is a HW PNETID on
the base, and different if there is none is IMHO wonky and again
asymmetric.

Imagine the following you have your nice little setup with a PNETID on
a non-leaf and a base_ndev that has no PNETID. Then your HW admin
configures a PNETID to your base_ndev, a different one. Suddenly
your ndev PNETID is ignored for reasons not obvious to you. Yes it is
similar to having a software PNETID on the base_ndev and getting it
overruled by a HW PNETID, but much less obvious IMHO. I also think
a software PNETID of the base should probably take precedence over over
the software pnetid of ndev.

Regards,
Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/1/14 20:07, Halil Pasic wrote:
> On Fri, 10 Jan 2025 13:43:44 +0800
> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
> 
>>> I think I showed a valid and practical setup that would break with your
>>> patch as is. Do you agree with that statement?  
>> Did you mean
>> "
>> Now for something like a bond of two OSA
>> interfaces, I would expect the two legs of the bond to probably have a
>> "HW PNETID", but the netdev representing the bond itself won't have one
>> unless the Linux admin defines a software PNETID, which is work, and
>> can't have a HW PNETID because it is a software construct within Linux.
>> Breaking for example an active-backup bond setup where the legs have
>> HW PNETIDs and the admin did not bother to specify a PNETID for the bond
>> is not acceptable.
>> " ?
>> If the legs have HW pnetids, add pnetid to bond netdev will fail as
>> smc_pnet_add_eth will check whether the base_ndev already have HW pnetid.
>>
>> If the legs without HW pnetids, and admin add pnetids to legs through smc_pnet.
>> Yes, my patch will break the setup. What Paolo suggests(both checking ndev and
>> base_ndev, and replace || by && )can help compatible with the setup.
> 
> I'm glad we agree on that part. Things are much more acceptable if we
> are doing both base and ndev. 
It is also acceptable for me.

> Nevertheless I would like to understand
> your problem better, and talk about it to my team. I will also ask some
> questions in another email.
Questions are welcome.

> 
> That said having things work differently if there is a HW PNETID on
> the base, and different if there is none is IMHO wonky and again
> asymmetric.
> 
> Imagine the following you have your nice little setup with a PNETID on
> a non-leaf and a base_ndev that has no PNETID. Then your HW admin
> configures a PNETID to your base_ndev, a different one. Suddenly
> your ndev PNETID is ignored for reasons not obvious to you. Yes it is
> similar to having a software PNETID on the base_ndev and getting it
> overruled by a HW PNETID, but much less obvious IMHO. I am wondering if there are any scenarios that require setting different
pnetids for different net devices in one netdev hierarchy. If no, maybe
we should limit that only one pnetid can be set to one netdev hierarchy.

> I also think
> a software PNETID of the base should probably take precedence over over
> the software pnetid of ndev.
Agree!

Thanks,
Guangguan Wang
> 
> Regards,
> Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/1/15 19:53, Guangguan Wang wrote:
> 
> 
> On 2025/1/14 20:07, Halil Pasic wrote:
>> On Fri, 10 Jan 2025 13:43:44 +0800
>> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
>>
>>>> I think I showed a valid and practical setup that would break with your
>>>> patch as is. Do you agree with that statement?  
>>> Did you mean
>>> "
>>> Now for something like a bond of two OSA
>>> interfaces, I would expect the two legs of the bond to probably have a
>>> "HW PNETID", but the netdev representing the bond itself won't have one
>>> unless the Linux admin defines a software PNETID, which is work, and
>>> can't have a HW PNETID because it is a software construct within Linux.
>>> Breaking for example an active-backup bond setup where the legs have
>>> HW PNETIDs and the admin did not bother to specify a PNETID for the bond
>>> is not acceptable.
>>> " ?
>>> If the legs have HW pnetids, add pnetid to bond netdev will fail as
>>> smc_pnet_add_eth will check whether the base_ndev already have HW pnetid.
>>>
>>> If the legs without HW pnetids, and admin add pnetids to legs through smc_pnet.
>>> Yes, my patch will break the setup. What Paolo suggests(both checking ndev and
>>> base_ndev, and replace || by && )can help compatible with the setup.
>>
>> I'm glad we agree on that part. Things are much more acceptable if we
>> are doing both base and ndev. 
> It is also acceptable for me.
> 
>> Nevertheless I would like to understand
>> your problem better, and talk about it to my team. I will also ask some
>> questions in another email.
> Questions are welcome.
> 
>>
>> That said having things work differently if there is a HW PNETID on
>> the base, and different if there is none is IMHO wonky and again
>> asymmetric.
>>
>> Imagine the following you have your nice little setup with a PNETID on
>> a non-leaf and a base_ndev that has no PNETID. Then your HW admin
>> configures a PNETID to your base_ndev, a different one. Suddenly
>> your ndev PNETID is ignored for reasons not obvious to you. Yes it is
>> similar to having a software PNETID on the base_ndev and getting it
>> overruled by a HW PNETID, but much less obvious IMHO. I am wondering if there are any scenarios that require setting different
> pnetids for different net devices in one netdev hierarchy. If no, maybe
> we should limit that only one pnetid can be set to one netdev hierarchy.
> 
>> I also think
>> a software PNETID of the base should probably take precedence over over
>> the software pnetid of ndev.
> Agree!
> 
> Thanks,
> Guangguan Wang
>>
>> Regards,
>> Halil

Hi Halil,

Are there any questions or further discussions about this patch? If no, I will
send a v2 patch, in which software pnetid will be searched in both base_ndev and ndev,
and base_ndev will take precedence over ndev.

Thanks,
Guangguan Wang

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Wenjia Zhang]

On 10.02.25 12:16, Guangguan Wang wrote:
> 
> 
> On 2025/1/15 19:53, Guangguan Wang wrote:
>>
>>
>> On 2025/1/14 20:07, Halil Pasic wrote:
>>> On Fri, 10 Jan 2025 13:43:44 +0800
>>> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
>>>
>>>>> I think I showed a valid and practical setup that would break with your
>>>>> patch as is. Do you agree with that statement?
>>>> Did you mean
>>>> "
>>>> Now for something like a bond of two OSA
>>>> interfaces, I would expect the two legs of the bond to probably have a
>>>> "HW PNETID", but the netdev representing the bond itself won't have one
>>>> unless the Linux admin defines a software PNETID, which is work, and
>>>> can't have a HW PNETID because it is a software construct within Linux.
>>>> Breaking for example an active-backup bond setup where the legs have
>>>> HW PNETIDs and the admin did not bother to specify a PNETID for the bond
>>>> is not acceptable.
>>>> " ?
>>>> If the legs have HW pnetids, add pnetid to bond netdev will fail as
>>>> smc_pnet_add_eth will check whether the base_ndev already have HW pnetid.
>>>>
>>>> If the legs without HW pnetids, and admin add pnetids to legs through smc_pnet.
>>>> Yes, my patch will break the setup. What Paolo suggests(both checking ndev and
>>>> base_ndev, and replace || by && )can help compatible with the setup.
>>>
>>> I'm glad we agree on that part. Things are much more acceptable if we
>>> are doing both base and ndev.
>> It is also acceptable for me.
>>
>>> Nevertheless I would like to understand
>>> your problem better, and talk about it to my team. I will also ask some
>>> questions in another email.
>> Questions are welcome.
>>
>>>
>>> That said having things work differently if there is a HW PNETID on
>>> the base, and different if there is none is IMHO wonky and again
>>> asymmetric.
>>>
>>> Imagine the following you have your nice little setup with a PNETID on
>>> a non-leaf and a base_ndev that has no PNETID. Then your HW admin
>>> configures a PNETID to your base_ndev, a different one. Suddenly
>>> your ndev PNETID is ignored for reasons not obvious to you. Yes it is
>>> similar to having a software PNETID on the base_ndev and getting it
>>> overruled by a HW PNETID, but much less obvious IMHO. I am wondering if there are any scenarios that require setting different
>> pnetids for different net devices in one netdev hierarchy. If no, maybe
>> we should limit that only one pnetid can be set to one netdev hierarchy.
>>
>>> I also think
>>> a software PNETID of the base should probably take precedence over over
>>> the software pnetid of ndev.
>> Agree!
>>
>> Thanks,
>> Guangguan Wang
>>>
>>> Regards,
>>> Halil
> 
> Hi Halil,
> 
> Are there any questions or further discussions about this patch? If no, I will
> send a v2 patch, in which software pnetid will be searched in both base_ndev and ndev,
> and base_ndev will take precedence over ndev.
> 
> Thanks,
> Guangguan Wang
> 
> 

Hi Guangguan,

Thank you for the detailed description and examples; I understand your 
situation better now. Paolo's suggestions (checking both ndev and 
base_ndev, and replacing || with &&) could indeed serve as a workaround 
for certain setups like yours. However, they might also introduce 
invalid topologies, one example is what Halil mentioned.

Therefore, neither suggestion is fully acceptable, whether from you or 
from Paolo. I agree that we should restrict it so that only one pnetid 
can be assigned to a single netdev hierarchy, based on the base ndev.

One preliminary idea I have is to enhance the smc_pnet -a -I <ethx> 
<pnetid> command to analyze the entire hierarchy first, ensuring that 
only one pnetid is assigned per netdev hierarchy.

Thanks,
Wenjia

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Fri, 10 Jan 2025 13:43:44 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> We want to use SMC in container on cloud environment, and encounter problem
> when using smc_pnet with commit 890a2cb4a966. In container, there have choices
> of different container network, such as directly using host network, virtual
> network IPVLAN, veth, etc. Different choices of container network have different
> netdev hierarchy. Examples of netdev hierarchy show below. (eth0 and eth1 in host
> below is the netdev directly related to the physical device).
>  _______________________________      ________________________________   
> |   _________________           |     |   _________________           |  
> |  |POD              |          |     |  |POD  __________  |          |  
> |  |                 |          |     |  |    |upper_ndev| |          |  
> |  | eth0_________   |          |     |  |eth0|__________| |          |  
> |  |____|         |__|          |     |  |_______|_________|          |  
> |       |         |             |     |          |lower netdev        |  
> |       |         |             |     |        __|______              |  
> |   eth1|base_ndev| eth0_______ |     |   eth1|         | eth0_______ |  
> |       |         |    | RDMA  ||     |       |base_ndev|    | RDMA  ||  
> | host  |_________|    |_______||     | host  |_________|    |_______||  
> ———————————————————————————————-      ———————————————————————————————-    
>  netdev hierarchy if directly          netdev hierarchy if using IPVLAN    
>    using host network
>  _______________________________
> |   _____________________       |
> |  |POD        _________ |      |
> |  |          |base_ndev||      |
> |  |eth0(veth)|_________||      |
> |  |____________|________|      |
> |               |pairs          |
> |        _______|_              |
> |       |         | eth0_______ |
> |   veth|base_ndev|    | RDMA  ||
> |       |_________|    |_______||
> |        _________              |
> |   eth1|base_ndev|             |
> | host  |_________|             |
>  ———————————————————————————————
>   netdev hierarchy if using veth
> 
> Due to some reasons, the eth1 in host is not RDMA attached netdevice, pnetid
> is needed to map the eth1(in host) with RDMA device so that POD can do SMC-R.
> Because the eth1(in host) is managed by CNI plugin(such as Terway, network
> management plugin in container environment), and in cloud environment the
> eth(in host) can dynamically be inserted by CNI when POD create and dynamically
> be removed by CNI when POD destroy and no POD related to the eth(in host)
> anymore.

I'm pretty clueless when it comes to the details of CNI but I think
I'm barely able to follow. Nevertheless if you have the feeling that
my extrapolations are wrong, please do point it out.

> It is hard for us to config the pnetid to the eth1(in host). So we
> config the pnetid to the netdevice which can be seen in POD.

Hm, this sounds like you could set PNETID on eth1 (in host) for each of
the cases and everything would be cool (and would work), but because CNI
and the environment do not support it, or supports it in a very
inconvenient way, you are looking for a workaround where PNETID is set
in the POD. Is that right? Or did I get something wrong?

> When do SMC-R, both
> the container directly using host network and the container using veth network
> can successfully match the RDMA device, because the configured pnetid netdev is a
> base_ndev. But the container using IPVLAN can not successfully match the RDMA
> device and 0x03030000 fallback happens, because the configured pnetid netdev is
> not a base_ndev. Additionally, if config pnetid to the eth1(in host) also can not
> work for matching RDMA device when using veth network and doing SMC-R in POD.

That I guess answers my question from the first paragraph. Setting
PNETID on eth1 (host) would not be sufficient for veth. Right?

Another silly question: is making the PNETID basically a part of the Pod
definition shifting PNETID from the realm of infrastructure (i.e.
configured by the cloud provider) to the ream of an application (i.e.
configured by the tenant)?

AFAIU veth (host) is bridged (or similar) to eth1 (host) and that is in
the host, and this is where we make sure that the requirements for SMC-R
are satisfied.

But veth (host) could be attached to eth3 which is on a network not
reachable via eth0 (host) or eth1 (host). In that case the pod could
still set PNETID on veth (POD). Or?

> 
> My patch can resolve the problem we encountered and also can unify the pnetid setup
> of different network choices list above, assuming the pnetid is not limited to
> config to the base_ndev directly related to the physical device(indeed, the current
> implementation has not limited it yet).

I see some problems here, but I'm afraid we see different problems. For
me not being able to set eth0 (veth/POD)'s PNEDID from the host is a
problem. Please notice that with the current implementation users can
only control the PNETID if infrastructure does not do so in the first
place.


Can you please help me reason about this? I'm unfortunately lacking
Kubernetes skills here, and it is difficult for me to think along.

Regards,
Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Wed, 15 Jan 2025 19:53:15 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> > Imagine the following you have your nice little setup with a PNETID on
> > a non-leaf and a base_ndev that has no PNETID. Then your HW admin
> > configures a PNETID to your base_ndev, a different one. Suddenly
> > your ndev PNETID is ignored for reasons not obvious to you. Yes it is
> > similar to having a software PNETID on the base_ndev and getting it
> > overruled by a HW PNETID, but much less obvious IMHO. I am wondering if there are any scenarios that require setting different  
> pnetids for different net devices in one netdev hierarchy. If no, maybe
> we should limit that only one pnetid can be set to one netdev hierarchy.

I wonder what topologies and changes to topologies are possible. If
changes to a topology are possible then making sure there is only one
PNETID within a netdev hierarchy can be difficult, as we would need
to prevent changing the topology if a device has a not PNETID. (E.g.
we first set a pnetid when the netdev is still not in a hierarchy
and then try to put it into the hierarchy that already has a different
PNETID within). Regarding allowable topologies, using your ASCII-art.

I think you could add 2 Pods with an IPVLAN eth0 (Pod) on top of eth1
(host) each. Those would be in a single hierarchy I guess, but I guess
you would still want to be able to set (most likely the same) PNETID
on each.

Bottom line is, this approach looks tricky to me. Maybe with a crisper
explanation on what are these upper-lower links for. Maybe I am
overgeneralizing here.

Regards,
Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Mon, 10 Feb 2025 19:16:57 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> Hi Halil,
> 
> Are there any questions or further discussions about this patch?

Hi Guangguan!

Sorry for taking so long. Yes I have asked some questions a couple of
minutes ago.

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/2/10 21:52, Halil Pasic wrote:
> On Fri, 10 Jan 2025 13:43:44 +0800
> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
> 
>> We want to use SMC in container on cloud environment, and encounter problem
>> when using smc_pnet with commit 890a2cb4a966. In container, there have choices
>> of different container network, such as directly using host network, virtual
>> network IPVLAN, veth, etc. Different choices of container network have different
>> netdev hierarchy. Examples of netdev hierarchy show below. (eth0 and eth1 in host
>> below is the netdev directly related to the physical device).
>>  _______________________________      ________________________________   
>> |   _________________           |     |   _________________           |  
>> |  |POD              |          |     |  |POD  __________  |          |  
>> |  |                 |          |     |  |    |upper_ndev| |          |  
>> |  | eth0_________   |          |     |  |eth0|__________| |          |  
>> |  |____|         |__|          |     |  |_______|_________|          |  
>> |       |         |             |     |          |lower netdev        |  
>> |       |         |             |     |        __|______              |  
>> |   eth1|base_ndev| eth0_______ |     |   eth1|         | eth0_______ |  
>> |       |         |    | RDMA  ||     |       |base_ndev|    | RDMA  ||  
>> | host  |_________|    |_______||     | host  |_________|    |_______||  
>> ———————————————————————————————-      ———————————————————————————————-    
>>  netdev hierarchy if directly          netdev hierarchy if using IPVLAN    
>>    using host network
>>  _______________________________
>> |   _____________________       |
>> |  |POD        _________ |      |
>> |  |          |base_ndev||      |
>> |  |eth0(veth)|_________||      |
>> |  |____________|________|      |
>> |               |pairs          |
>> |        _______|_              |
>> |       |         | eth0_______ |
>> |   veth|base_ndev|    | RDMA  ||
>> |       |_________|    |_______||
>> |        _________              |
>> |   eth1|base_ndev|             |
>> | host  |_________|             |
>>  ———————————————————————————————
>>   netdev hierarchy if using veth
>>
>> Due to some reasons, the eth1 in host is not RDMA attached netdevice, pnetid
>> is needed to map the eth1(in host) with RDMA device so that POD can do SMC-R.
>> Because the eth1(in host) is managed by CNI plugin(such as Terway, network
>> management plugin in container environment), and in cloud environment the
>> eth(in host) can dynamically be inserted by CNI when POD create and dynamically
>> be removed by CNI when POD destroy and no POD related to the eth(in host)
>> anymore.
> 
> I'm pretty clueless when it comes to the details of CNI but I think
> I'm barely able to follow. Nevertheless if you have the feeling that
> my extrapolations are wrong, please do point it out.
> 
>> It is hard for us to config the pnetid to the eth1(in host). So we
>> config the pnetid to the netdevice which can be seen in POD.
> 
> Hm, this sounds like you could set PNETID on eth1 (in host) for each of
> the cases and everything would be cool (and would work), but because CNI
> and the environment do not support it, or supports it in a very
> inconvenient way, you are looking for a workaround where PNETID is set
> in the POD. Is that right? Or did I get something wrong?

Right.

> 
>> When do SMC-R, both
>> the container directly using host network and the container using veth network
>> can successfully match the RDMA device, because the configured pnetid netdev is a
>> base_ndev. But the container using IPVLAN can not successfully match the RDMA
>> device and 0x03030000 fallback happens, because the configured pnetid netdev is
>> not a base_ndev. Additionally, if config pnetid to the eth1(in host) also can not
>> work for matching RDMA device when using veth network and doing SMC-R in POD.
> 
> That I guess answers my question from the first paragraph. Setting
> PNETID on eth1 (host) would not be sufficient for veth. Right?

Right. It is also one of the reasons for setting PNETID in POD.

> 
> Another silly question: is making the PNETID basically a part of the Pod
> definition shifting PNETID from the realm of infrastructure (i.e.
> configured by the cloud provider) to the ream of an application (i.e.
> configured by the tenant)?

No, application do not need to know the PNETID configuration. We have a plugin in
Kubernetes. When deploying a POD, the plugin will automatically add an initContainer
to the POD and automatically configure the PNETID in initContainer.

> 
> AFAIU veth (host) is bridged (or similar) to eth1 (host) and that is in
> the host, and this is where we make sure that the requirements for SMC-R
> are satisfied.
> 
> But veth (host) could be attached to eth3 which is on a network not
> reachable via eth0 (host) or eth1 (host). In that case the pod could
> still set PNETID on veth (POD). Or?
> 

Sorry, I forget to add a precondition, it is a single-tenant scenario, and all of the
ethX in host are in the same VPC(A term in Cloud, can be simply understood as a private
network domain). The ethX in the same VPC means they have the same network reachability.
Therefore, in this scenario, we will not encounter the situation you mentioned.

>>
>> My patch can resolve the problem we encountered and also can unify the pnetid setup
>> of different network choices list above, assuming the pnetid is not limited to
>> config to the base_ndev directly related to the physical device(indeed, the current
>> implementation has not limited it yet).
> 
> I see some problems here, but I'm afraid we see different problems. For
> me not being able to set eth0 (veth/POD)'s PNEDID from the host is a
> problem. Please notice that with the current implementation users can
> only control the PNETID if infrastructure does not do so in the first
> place.
> 
> 
> Can you please help me reason about this? I'm unfortunately lacking
> Kubernetes skills here, and it is difficult for me to think along.

Yes, it is also a problem that not being able to set eth0 (veth/POD)'s PNEDID from the host.
Even if the eth1(host) have hardware PNETID, the eth0 (veth/POD) can not search the hardware
PNETID. Because the eth0 (veth/POD) and eth1(host) are not in one netdev hierarchy.
But the two netdev hierarchies have relationship. Maybe search PNETID in all related netdev
hierarchies can help resolve this. For example when finding the base_ndev, if the base_ndev
is a netdev has relationship with other netdev(veth .etc) then jump to the related netdev
hierarchy through the relationship to iteratively find the base_ndev.
It is an idea now. I have not do any research about it yet and I am not sure if it is feasible.

Thanks,
Guangguan Wang

> 
> Regards,
> Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Halil Pasic]

On Tue, 11 Feb 2025 11:44:32 +0800
Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:

> > Can you please help me reason about this? I'm unfortunately lacking
> > Kubernetes skills here, and it is difficult for me to think along.  
> 
> Yes, it is also a problem that not being able to set eth0 (veth/POD)'s PNEDID from the host.
> Even if the eth1(host) have hardware PNETID, the eth0 (veth/POD) can not search the hardware
> PNETID. Because the eth0 (veth/POD) and eth1(host) are not in one netdev hierarchy.
> But the two netdev hierarchies have relationship. Maybe search PNETID in all related netdev
> hierarchies can help resolve this. For example when finding the base_ndev, if the base_ndev
> is a netdev has relationship with other netdev(veth .etc) then jump to the related netdev
> hierarchy through the relationship to iteratively find the base_ndev.
> It is an idea now. I have not do any research about it yet and I am not sure if it is feasible.

I did a fair amount of thinking and I've talked to Wenjia and Sandy as
well, and now I'm fine with moving forward with a variant that
prioritizes compatibility but makes the scenarios you have pointed out
work by enabling taking the SW PNETID of the non-leaf netdev(s) if the
base_dev has no PNETID (neither hw nor sw).

Regards,
Halil

Re: [PATCH net] net/smc: use the correct ndev to find pnetid by pnetid table

[Guangguan Wang]

On 2025/3/3 22:24, Halil Pasic wrote:
> On Tue, 11 Feb 2025 11:44:32 +0800
> Guangguan Wang <guangguan.wang@linux.alibaba.com> wrote:
> 
>>> Can you please help me reason about this? I'm unfortunately lacking
>>> Kubernetes skills here, and it is difficult for me to think along.  
>>
>> Yes, it is also a problem that not being able to set eth0 (veth/POD)'s PNEDID from the host.
>> Even if the eth1(host) have hardware PNETID, the eth0 (veth/POD) can not search the hardware
>> PNETID. Because the eth0 (veth/POD) and eth1(host) are not in one netdev hierarchy.
>> But the two netdev hierarchies have relationship. Maybe search PNETID in all related netdev
>> hierarchies can help resolve this. For example when finding the base_ndev, if the base_ndev
>> is a netdev has relationship with other netdev(veth .etc) then jump to the related netdev
>> hierarchy through the relationship to iteratively find the base_ndev.
>> It is an idea now. I have not do any research about it yet and I am not sure if it is feasible.
> 
> I did a fair amount of thinking and I've talked to Wenjia and Sandy as
> well, and now I'm fine with moving forward with a variant that
> prioritizes compatibility but makes the scenarios you have pointed out
> work by enabling taking the SW PNETID of the non-leaf netdev(s) if the
> base_dev has no PNETID (neither hw nor sw).
> 
> Regards,
> Halil

Thanks Halil, Wenjia and Alexandra.

I will send a v2 patch soon, in which software pnetid will be searched in both base_ndev and ndev,
and base_ndev will take precedence over ndev.

Regards,
Guangguan Wang