| Message ID | 20230329031337.3444547-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bonding: add software timestamping support | expand |
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
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.
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
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
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.
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
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
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
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.
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
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);
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(-)