[PATCH net-next] bonding: add software timestamping support

[PATCH net-next] bonding: add software timestamping support

[Hangbin Liu]

At present, bonding attempts to obtain the timestamp (ts) information of
the active slave. However, this feature is only available for mode 1, 5,
and 6. For other modes, bonding doesn't even provide support for software
timestamping. To address this issue, let's call ethtool_op_get_ts_info
when there is no primary active slave. This will enable the use of software
timestamping for the bonding interface.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 00646aa315c3..f0856bec59f5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5709,9 +5709,7 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
 		}
 	}
 
-	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
-				SOF_TIMESTAMPING_SOFTWARE;
-	info->phc_index = -1;
+	ret = ethtool_op_get_ts_info(bond_dev, info);
 
 out:
 	dev_put(real_dev);
-- 
2.38.1

Re: [PATCH net-next] bonding: add software timestamping support

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>At present, bonding attempts to obtain the timestamp (ts) information of
>the active slave. However, this feature is only available for mode 1, 5,
>and 6. For other modes, bonding doesn't even provide support for software
>timestamping. To address this issue, let's call ethtool_op_get_ts_info
>when there is no primary active slave. This will enable the use of software
>timestamping for the bonding interface.

	If I'm reading the patch below correctly, the actual functional
change here is to additionally set SOF_TIMESTAMPING_TX_SOFTWARE in
so_timestamping for the active-backup, balance-tlb and balance-alb modes
(or whenever there's no active slave, but that's less interesting).  So,
this patch only changes the behavior with regards to transmit
timestamping, correct?

	I'm not objecting to the patch per se, but the description above
does not appear to correctly describe the change.

	-J

>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_main.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 00646aa315c3..f0856bec59f5 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -5709,9 +5709,7 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
> 		}
> 	}
> 
>-	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
>-				SOF_TIMESTAMPING_SOFTWARE;
>-	info->phc_index = -1;
>+	ret = ethtool_op_get_ts_info(bond_dev, info);
> 
> out:
> 	dev_put(real_dev);
>-- 
>2.38.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net-next] bonding: add software timestamping support

[Hangbin Liu]

On Tue, Mar 28, 2023 at 08:36:58PM -0700, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >At present, bonding attempts to obtain the timestamp (ts) information of
> >the active slave. However, this feature is only available for mode 1, 5,
> >and 6. For other modes, bonding doesn't even provide support for software
> >timestamping. To address this issue, let's call ethtool_op_get_ts_info
> >when there is no primary active slave. This will enable the use of software
> >timestamping for the bonding interface.
> 
> 	If I'm reading the patch below correctly, the actual functional
> change here is to additionally set SOF_TIMESTAMPING_TX_SOFTWARE in
> so_timestamping for the active-backup, balance-tlb and balance-alb modes

No. In the description. I said for other modes, bonding doesn't even provide
support for software timestamping. So this patch is to address this issue.
i.e. add sw timestaming for all bonding modes.

For mode 1,5,6. We will try find the active slave and get it's ts info
directly. If there is no ops->get_ts_info, just use sw timestamping.

For other modes, use sw timestamping directly.

This is because some users want to use PTP over bond with other modes. e.g. LACP.
They are satisfied with just sw timestamping as it's difficult to support hw
timestamping for LACP bonding.

Before this patch, bond mode with 0, 2, 3, 4 only has software-receive.

# ethtool -T bond0
Time stamping parameters for bond0:
Capabilities:
        software-receive
        software-system-clock
PTP Hardware Clock: none
Hardware Transmit Timestamp Modes: none
Hardware Receive Filter Modes: none

# ptp4l -m -S -i bond0
ptp4l[66296.154]: interface 'bond0' does not support requested timestamping mode
failed to create a clock

After this patch:

# ethtool -T bond0
Time stamping parameters for bond0:
Capabilities:
        software-transmit
        software-receive
        software-system-clock
PTP Hardware Clock: none
Hardware Transmit Timestamp Modes: none
Hardware Receive Filter Modes: none

# ptp4l -m -S -i bond0
ptp4l[66952.474]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[66952.474]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[66952.474]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[66981.681]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[66981.681]: selected local clock 007c50.fffe.70cdb6 as best master
ptp4l[66981.682]: port 1: assuming the grand master role

Thanks
Hangbin

Re: [PATCH net-next] bonding: add software timestamping support

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Tue, Mar 28, 2023 at 08:36:58PM -0700, Jay Vosburgh wrote:
>> Hangbin Liu <liuhangbin@gmail.com> wrote:
>> 
>> >At present, bonding attempts to obtain the timestamp (ts) information of
>> >the active slave. However, this feature is only available for mode 1, 5,
>> >and 6. For other modes, bonding doesn't even provide support for software
>> >timestamping. To address this issue, let's call ethtool_op_get_ts_info
>> >when there is no primary active slave. This will enable the use of software
>> >timestamping for the bonding interface.
>> 
>> 	If I'm reading the patch below correctly, the actual functional
>> change here is to additionally set SOF_TIMESTAMPING_TX_SOFTWARE in
>> so_timestamping for the active-backup, balance-tlb and balance-alb modes
>
>No. In the description. I said for other modes, bonding doesn't even provide
>support for software timestamping. So this patch is to address this issue.
>i.e. add sw timestaming for all bonding modes.

	Ok, I think I follow now.  It is still adding only TX software
timestamping, as (from your example below) RX was already available.

	So, I do think the patch description is imprecise in saying,
"For other modes, bonding doesn't even provide support for software
timestamping" as this really refers to specifically TX timestamping.

	-J

>For mode 1,5,6. We will try find the active slave and get it's ts info
>directly. If there is no ops->get_ts_info, just use sw timestamping.
>
>For other modes, use sw timestamping directly.
>
>This is because some users want to use PTP over bond with other modes. e.g. LACP.
>They are satisfied with just sw timestamping as it's difficult to support hw
>timestamping for LACP bonding.
>
>Before this patch, bond mode with 0, 2, 3, 4 only has software-receive.
>
># ethtool -T bond0
>Time stamping parameters for bond0:
>Capabilities:
>        software-receive
>        software-system-clock
>PTP Hardware Clock: none
>Hardware Transmit Timestamp Modes: none
>Hardware Receive Filter Modes: none
>
># ptp4l -m -S -i bond0
>ptp4l[66296.154]: interface 'bond0' does not support requested timestamping mode
>failed to create a clock
>
>After this patch:
>
># ethtool -T bond0
>Time stamping parameters for bond0:
>Capabilities:
>        software-transmit
>        software-receive
>        software-system-clock
>PTP Hardware Clock: none
>Hardware Transmit Timestamp Modes: none
>Hardware Receive Filter Modes: none
>
># ptp4l -m -S -i bond0
>ptp4l[66952.474]: port 1: INITIALIZING to LISTENING on INIT_COMPLETE
>ptp4l[66952.474]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
>ptp4l[66952.474]: port 0: INITIALIZING to LISTENING on INIT_COMPLETE
>ptp4l[66981.681]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
>ptp4l[66981.681]: selected local clock 007c50.fffe.70cdb6 as best master
>ptp4l[66981.682]: port 1: assuming the grand master role
>
>Thanks
>Hangbin

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net-next] bonding: add software timestamping support

[Miroslav Lichvar]

On Wed, Mar 29, 2023 at 11:13:37AM +0800, Hangbin Liu wrote:
> At present, bonding attempts to obtain the timestamp (ts) information of
> the active slave. However, this feature is only available for mode 1, 5,
> and 6. For other modes, bonding doesn't even provide support for software
> timestamping. To address this issue, let's call ethtool_op_get_ts_info
> when there is no primary active slave. This will enable the use of software
> timestamping for the bonding interface.

Would it make sense to check if all devices in the bond support
SOF_TIMESTAMPING_TX_SOFTWARE before returning it for the bond?
Applications might expect that a SW TX timestamp will be always
provided if the capability is reported.

-- 
Miroslav Lichvar

Re: [PATCH net-next] bonding: add software timestamping support

[Hangbin Liu]

On Wed, Mar 29, 2023 at 12:27:11PM +0200, Miroslav Lichvar wrote:
> On Wed, Mar 29, 2023 at 11:13:37AM +0800, Hangbin Liu wrote:
> > At present, bonding attempts to obtain the timestamp (ts) information of
> > the active slave. However, this feature is only available for mode 1, 5,
> > and 6. For other modes, bonding doesn't even provide support for software
> > timestamping. To address this issue, let's call ethtool_op_get_ts_info
> > when there is no primary active slave. This will enable the use of software
> > timestamping for the bonding interface.
> 
> Would it make sense to check if all devices in the bond support
> SOF_TIMESTAMPING_TX_SOFTWARE before returning it for the bond?
> Applications might expect that a SW TX timestamp will be always
> provided if the capability is reported.

In my understanding this is a software feature, no need for hardware support.
In __sock_tx_timestamp() it will set skb tx_flags when we have
SOF_TIMESTAMPING_TX_SOFTWARE flag. Do I understand wrong?

Thanks
Hangbin

Re: [PATCH net-next] bonding: add software timestamping support

[Jakub Kicinski]

On Thu, 30 Mar 2023 11:33:43 +0800 Hangbin Liu wrote:
> On Wed, Mar 29, 2023 at 12:27:11PM +0200, Miroslav Lichvar wrote:
> > On Wed, Mar 29, 2023 at 11:13:37AM +0800, Hangbin Liu wrote:  
> > > At present, bonding attempts to obtain the timestamp (ts) information of
> > > the active slave. However, this feature is only available for mode 1, 5,
> > > and 6. For other modes, bonding doesn't even provide support for software
> > > timestamping. To address this issue, let's call ethtool_op_get_ts_info
> > > when there is no primary active slave. This will enable the use of software
> > > timestamping for the bonding interface.  
> > 
> > Would it make sense to check if all devices in the bond support
> > SOF_TIMESTAMPING_TX_SOFTWARE before returning it for the bond?
> > Applications might expect that a SW TX timestamp will be always
> > provided if the capability is reported.  
> 
> In my understanding this is a software feature, no need for hardware support.
> In __sock_tx_timestamp() it will set skb tx_flags when we have
> SOF_TIMESTAMPING_TX_SOFTWARE flag. Do I understand wrong?

Driver needs to call skb_tx_timestamp(), so unlike with Rx there's
something to do for the driver.

Re: [PATCH net-next] bonding: add software timestamping support

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Wed, Mar 29, 2023 at 12:27:11PM +0200, Miroslav Lichvar wrote:
>> On Wed, Mar 29, 2023 at 11:13:37AM +0800, Hangbin Liu wrote:
>> > At present, bonding attempts to obtain the timestamp (ts) information of
>> > the active slave. However, this feature is only available for mode 1, 5,
>> > and 6. For other modes, bonding doesn't even provide support for software
>> > timestamping. To address this issue, let's call ethtool_op_get_ts_info
>> > when there is no primary active slave. This will enable the use of software
>> > timestamping for the bonding interface.
>> 
>> Would it make sense to check if all devices in the bond support
>> SOF_TIMESTAMPING_TX_SOFTWARE before returning it for the bond?
>> Applications might expect that a SW TX timestamp will be always
>> provided if the capability is reported.
>
>In my understanding this is a software feature, no need for hardware support.
>In __sock_tx_timestamp() it will set skb tx_flags when we have
>SOF_TIMESTAMPING_TX_SOFTWARE flag. Do I understand wrong?

	Right, but the network device driver is required to call
skb_tx_timestamp() in order to record the actual timestamp for the
software timestamping case.

	Do all drivers that may be members of a bond return
SOF_TIMESTAMPING_TX_SOFTWARE to .get_ts_info and properly call
skb_tx_timestamp()?  I.e., is this something that needs to be checked,
or is it safe to assume it's always true?

	If I'm reading things correctly, the answer is no, as one
exception appears to be IPOIB, which doesn't define .get_ts_info that I
can find, and does not call skb_tx_timestamp() in ipoib_start_xmit().

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net-next] bonding: add software timestamping support

[Hangbin Liu]

On Wed, Mar 29, 2023 at 09:12:44PM -0700, Jay Vosburgh wrote:
> >> Would it make sense to check if all devices in the bond support
> >> SOF_TIMESTAMPING_TX_SOFTWARE before returning it for the bond?
> >> Applications might expect that a SW TX timestamp will be always
> >> provided if the capability is reported.
> >
> >In my understanding this is a software feature, no need for hardware support.
> >In __sock_tx_timestamp() it will set skb tx_flags when we have
> >SOF_TIMESTAMPING_TX_SOFTWARE flag. Do I understand wrong?
> 
> 	Right, but the network device driver is required to call
> skb_tx_timestamp() in order to record the actual timestamp for the
> software timestamping case.
> 
> 	Do all drivers that may be members of a bond return
> SOF_TIMESTAMPING_TX_SOFTWARE to .get_ts_info and properly call
> skb_tx_timestamp()?  I.e., is this something that needs to be checked,
> or is it safe to assume it's always true?
> 
> 	If I'm reading things correctly, the answer is no, as one
> exception appears to be IPOIB, which doesn't define .get_ts_info that I
> CAN Find, and does not call skb_tx_timestamp() in ipoib_start_xmit().

Oh.. I thought it's a software timestamp and all driver's should support it.
I didn't expect that Infiniband doesn't support it. Based on this, it seems
we can't even assume that all Ethernet drivers will support it, since a
private driver may also not call skb_tx_timestamp() during transmit. Even if
we check the slaves during ioctl call, we can't expect a later-joined slave
to have SW TX timestamp support. It seems that we'll have to drop this feature."

Thanks
Hangbin

Re: [PATCH net-next] bonding: add software timestamping support

[Miroslav Lichvar]

On Fri, Mar 31, 2023 at 11:32:03AM +0800, Hangbin Liu wrote:
> On Wed, Mar 29, 2023 at 09:12:44PM -0700, Jay Vosburgh wrote:
> > 	If I'm reading things correctly, the answer is no, as one
> > exception appears to be IPOIB, which doesn't define .get_ts_info that I
> > CAN Find, and does not call skb_tx_timestamp() in ipoib_start_xmit().
> 
> Oh.. I thought it's a software timestamp and all driver's should support it.
> I didn't expect that Infiniband doesn't support it. Based on this, it seems
> we can't even assume that all Ethernet drivers will support it, since a
> private driver may also not call skb_tx_timestamp() during transmit. Even if
> we check the slaves during ioctl call, we can't expect a later-joined slave
> to have SW TX timestamp support. It seems that we'll have to drop this feature."

I'd not see that as a problem. At the time of the ioctl call the
information is valid. I think knowing that some timestamps will be
missing due to an interface not supporting the feature is a different
case than the admin later adding a new interface to the bond and
breaking the condition. The application likely already have some
expectations after it starts and configures timestamping, e.g. that
the RX filter is not changed or TX timestamping disabled.

-- 
Miroslav Lichvar

Re: [PATCH net-next] bonding: add software timestamping support

[Hangbin Liu]

On Mon, Apr 03, 2023 at 12:18:03PM +0200, Miroslav Lichvar wrote:
> > Oh.. I thought it's a software timestamp and all driver's should support it.
> > I didn't expect that Infiniband doesn't support it. Based on this, it seems
> > we can't even assume that all Ethernet drivers will support it, since a
> > private driver may also not call skb_tx_timestamp() during transmit. Even if
> > we check the slaves during ioctl call, we can't expect a later-joined slave
> > to have SW TX timestamp support. It seems that we'll have to drop this feature."
> 
> I'd not see that as a problem. At the time of the ioctl call the
> information is valid. I think knowing that some timestamps will be
> missing due to an interface not supporting the feature is a different
> case than the admin later adding a new interface to the bond and
> breaking the condition. The application likely already have some
> expectations after it starts and configures timestamping, e.g. that
> the RX filter is not changed or TX timestamping disabled.

Thanks, this makes sense to me. I will try this way and post the new patch.

Hangbin