| Message ID | 20220403175544.26556-5-gerhard@engleder-embedded.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Support hardware clocks with additional free running cycle counter | expand |
On Sun, Apr 03, 2022 at 07:55:43PM +0200, Gerhard Engleder wrote: > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > if (shhwtstamps && > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > !skb_is_swtx_tstamp(skb, false_tstamp)) { > + rcu_read_lock(); > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); __sock_recv_timestamp() is hot path. No need to call dev_get_by_napi_id() for the vast majority of cases using plain old MAC time stamping. Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). Thanks, Richard > + if (orig_dev) { > + if_index = orig_dev->ifindex; > + hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps, > + sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC); > + } else { > + if_index = 0; > + hwtstamp = shhwtstamps->hwtstamp; > + } > + rcu_read_unlock(); > + > if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) > - hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp, > + hwtstamp = ptp_convert_timestamp(&hwtstamp, > sk->sk_bind_phc); > - else > - hwtstamp = shhwtstamps->hwtstamp; > > if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) { > empty = 0; > > if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) && > !skb_is_err_queue(skb)) > - put_ts_pktinfo(msg, skb); > + put_ts_pktinfo(msg, skb, if_index); > } > } > if (!empty) { > -- > 2.20.1 >
> > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > > if (shhwtstamps && > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > > !skb_is_swtx_tstamp(skb, false_tstamp)) { > > + rcu_read_lock(); > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > > __sock_recv_timestamp() is hot path. > > No need to call dev_get_by_napi_id() for the vast majority of cases > using plain old MAC time stamping. Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()? That's the reason for the removal of a separate flag, which signals the need to timestamp determination based on address/cookie. I thought there is no need for that flag, as netdev is already available later in the existing code. > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). This flag tells netdev_get_tstamp() which timestamp is required. If it is not set, then netdev_get_tstamp() has to deliver the normal timestamp as always. But this normal timestamp is only available via address/cookie. So netdev_get_tstamp() must be called. Thank you! Gerhard
On Sun, Apr 10, 2022 at 02:54:36PM +0200, Gerhard Engleder wrote: > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > > > if (shhwtstamps && > > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > > > !skb_is_swtx_tstamp(skb, false_tstamp)) { > > > + rcu_read_lock(); > > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > > > > __sock_recv_timestamp() is hot path. > > > > No need to call dev_get_by_napi_id() for the vast majority of cases > > using plain old MAC time stamping. > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()? No. Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested. > That's the reason for the removal of a separate flag, which signals the need to > timestamp determination based on address/cookie. I thought there is no need > for that flag, as netdev is already available later in the existing code. > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). > > This flag tells netdev_get_tstamp() which timestamp is required. If it > is not set, then > netdev_get_tstamp() has to deliver the normal timestamp as always. But > this normal > timestamp is only available via address/cookie. So netdev_get_tstamp() must be > called. It should be this: - normal, non-vclock: use hwtstamps->hwtstamp directly - vclock: use slower path with lookup I don't see why you can't implement that. Thanks, Richard
> > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > > > > if (shhwtstamps && > > > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > > > > !skb_is_swtx_tstamp(skb, false_tstamp)) { > > > > + rcu_read_lock(); > > > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > > > > > > __sock_recv_timestamp() is hot path. > > > > > > No need to call dev_get_by_napi_id() for the vast majority of cases > > > using plain old MAC time stamping. > > > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()? > > No. Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested. You are right, my fault. > > That's the reason for the removal of a separate flag, which signals the need to > > timestamp determination based on address/cookie. I thought there is no need > > for that flag, as netdev is already available later in the existing code. > > > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). > > > > This flag tells netdev_get_tstamp() which timestamp is required. If it > > is not set, then > > netdev_get_tstamp() has to deliver the normal timestamp as always. But > > this normal > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be > > called. > > It should be this: > > - normal, non-vclock: use hwtstamps->hwtstamp directly > - vclock: use slower path with lookup > > I don't see why you can't implement that. I will try to implement it that way. Thank you! Gerhard
> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > > > > > if (shhwtstamps && > > > > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > > > > > !skb_is_swtx_tstamp(skb, false_tstamp)) { > > > > > + rcu_read_lock(); > > > > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > > > > > > > > __sock_recv_timestamp() is hot path. > > > > > > > > No need to call dev_get_by_napi_id() for the vast majority of cases > > > > using plain old MAC time stamping. > > > > > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()? > > > > No. Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested. > > You are right, my fault. > > > > That's the reason for the removal of a separate flag, which signals the need to > > > timestamp determination based on address/cookie. I thought there is no need > > > for that flag, as netdev is already available later in the existing code. > > > > > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). > > > > > > This flag tells netdev_get_tstamp() which timestamp is required. If it > > > is not set, then > > > netdev_get_tstamp() has to deliver the normal timestamp as always. But > > > this normal > > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be > > > called. > > > > It should be this: > > > > - normal, non-vclock: use hwtstamps->hwtstamp directly > > - vclock: use slower path with lookup > > > > I don't see why you can't implement that. > > I will try to implement it that way. I'm thinking about why there should be a slow path with lookup. If the address/cookie points to a defined data structure with two timestamps, then no lookup for the phc or netdev is necessary. It should be possible for every driver to allocate a skbuff with enough space for this structure in front of the received Ethernet frame. The structure could be: struct skb_inline_hwtstamps { ktime_t hwtstamp; ktime_t hwcstamp; }; Actually my device and igc are storing the timestamps in front of the received Ethernet frame. In my opinion it is obvious to the store metadata of received Ethernet frames in front of it, because it eliminates the need for another DMA transfer. What is your opinion Vinicius? Gerhard
Gerhard Engleder <gerhard@engleder-embedded.com> writes: >> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, >> > > > > if (shhwtstamps && >> > > > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && >> > > > > !skb_is_swtx_tstamp(skb, false_tstamp)) { >> > > > > + rcu_read_lock(); >> > > > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); >> > > > >> > > > __sock_recv_timestamp() is hot path. >> > > > >> > > > No need to call dev_get_by_napi_id() for the vast majority of cases >> > > > using plain old MAC time stamping. >> > > >> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()? >> > >> > No. Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested. >> >> You are right, my fault. >> >> > > That's the reason for the removal of a separate flag, which signals the need to >> > > timestamp determination based on address/cookie. I thought there is no need >> > > for that flag, as netdev is already available later in the existing code. >> > > >> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). >> > > >> > > This flag tells netdev_get_tstamp() which timestamp is required. If it >> > > is not set, then >> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But >> > > this normal >> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be >> > > called. >> > >> > It should be this: >> > >> > - normal, non-vclock: use hwtstamps->hwtstamp directly >> > - vclock: use slower path with lookup >> > >> > I don't see why you can't implement that. >> >> I will try to implement it that way. > > I'm thinking about why there should be a slow path with lookup. If the > address/cookie > points to a defined data structure with two timestamps, then no lookup > for the phc or > netdev is necessary. It should be possible for every driver to > allocate a skbuff with enough > space for this structure in front of the received Ethernet frame. The > structure could be: > > struct skb_inline_hwtstamps { > ktime_t hwtstamp; > ktime_t hwcstamp; > }; > > Actually my device and igc are storing the timestamps in front of the > received Ethernet > frame. In my opinion it is obvious to the store metadata of received > Ethernet frames in > front of it, because it eliminates the need for another DMA transfer. > What is your opinion > Vinicius? If I am understanding this right, the idea is providing both "cycles" (free running cycles measurement) and PHC timestamp for all packets, for igc, it will work fine for RX (the HW already writes the timestamps for two timer registers in the host memory), but for TX it's going be awkward/slow (I would have to read two extra registers), but I think it's still possible. But it would be best to avoid the overhead, and only providing the "extra" (the cycles one) measurement if necessary for TX, so SKBTX_HW_TSTAMP_USE_CYCLES would still be needed. So, in short, I am fine with it, as long as I can get away with only providing the cycles measurement for TX if necessary. > > Gerhard Cheers,
On Tue, Apr 12, 2022 at 09:24:10PM +0200, Gerhard Engleder wrote: > I'm thinking about why there should be a slow path with lookup. If the > address/cookie > points to a defined data structure with two timestamps, then no lookup > for the phc or > netdev is necessary. It should be possible for every driver to > allocate a skbuff with enough > space for this structure in front of the received Ethernet frame. Adding 16 bytes for every allocated skbuff is going to be a tough sell. Most people don't want/need this. Thanks, Richard
> >> > > > > @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > >> > > > > if (shhwtstamps && > >> > > > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > >> > > > > !skb_is_swtx_tstamp(skb, false_tstamp)) { > >> > > > > + rcu_read_lock(); > >> > > > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > >> > > > > >> > > > __sock_recv_timestamp() is hot path. > >> > > > > >> > > > No need to call dev_get_by_napi_id() for the vast majority of cases > >> > > > using plain old MAC time stamping. > >> > > > >> > > Isn't dev_get_by_napi_id() called most of the time anyway in put_ts_pktinfo()? > >> > > >> > No. Only when SOF_TIMESTAMPING_OPT_PKTINFO is requested. > >> > >> You are right, my fault. > >> > >> > > That's the reason for the removal of a separate flag, which signals the need to > >> > > timestamp determination based on address/cookie. I thought there is no need > >> > > for that flag, as netdev is already available later in the existing code. > >> > > > >> > > > Make this conditional on (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC). > >> > > > >> > > This flag tells netdev_get_tstamp() which timestamp is required. If it > >> > > is not set, then > >> > > netdev_get_tstamp() has to deliver the normal timestamp as always. But > >> > > this normal > >> > > timestamp is only available via address/cookie. So netdev_get_tstamp() must be > >> > > called. > >> > > >> > It should be this: > >> > > >> > - normal, non-vclock: use hwtstamps->hwtstamp directly > >> > - vclock: use slower path with lookup > >> > > >> > I don't see why you can't implement that. > >> > >> I will try to implement it that way. > > > > I'm thinking about why there should be a slow path with lookup. If the > > address/cookie > > points to a defined data structure with two timestamps, then no lookup > > for the phc or > > netdev is necessary. It should be possible for every driver to > > allocate a skbuff with enough > > space for this structure On Tue, Apr 12, 2022 at 11:05 PM Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote: > > Gerhard Engleder <gerhard@engleder-embedded.com> writes: > in front of the received Ethernet frame. The > > structure could be: > > > > struct skb_inline_hwtstamps { > > ktime_t hwtstamp; > > ktime_t hwcstamp; > > }; > > > > Actually my device and igc are storing the timestamps in front of the > > received Ethernet > > frame. In my opinion it is obvious to the store metadata of received > > Ethernet frames in > > front of it, because it eliminates the need for another DMA transfer. > > What is your opinion > > Vinicius? > > If I am understanding this right, the idea is providing both "cycles" > (free running cycles measurement) and PHC timestamp for all packets, for > igc, it will work fine for RX (the HW already writes the timestamps for > two timer registers in the host memory), but for TX it's going be > awkward/slow (I would have to read two extra registers), but I think > it's still possible. > > But it would be best to avoid the overhead, and only providing the > "extra" (the cycles one) measurement if necessary for TX, so > SKBTX_HW_TSTAMP_USE_CYCLES would still be needed. > > So, in short, I am fine with it, as long as I can get away with only > providing the cycles measurement for TX if necessary. Of course for TX only cycles measurement shall be provided and SKBTX_HW_TSTAMP_USE_CYCLES is used for that. Thanks! Gerhard
> > I'm thinking about why there should be a slow path with lookup. If the > > address/cookie > > points to a defined data structure with two timestamps, then no lookup > > for the phc or > > netdev is necessary. It should be possible for every driver to > > allocate a skbuff with enough > > space for this structure in front of the received Ethernet frame. > > Adding 16 bytes for every allocated skbuff is going to be a tough > sell. Most people don't want/need this. Most people are not affected because they use drivers which do not support cycles. Those drivers stay the same, no 16 bytes are added. For TX those 16 bytes are not added, because SKBTX_HW_TSTAMP_USE_CYCLES is used to fill in the right time stamp. For igc and tsnep the 16 bytes in front of the RX frame exist anyway. So this would be a minimal solution with best performance as a first step. A lookup for netdev/phc can be added in the future if there is a driver, which needs that. Is it worth posting an implementation in that direction? Thanks! Gerhard
On Wed, Apr 13, 2022 at 10:51:54PM +0200, Gerhard Engleder wrote: > For igc and tsnep the 16 bytes in front of the RX frame exist anyway. > So this would be a minimal solution with best performance as a first > step. A lookup for netdev/phc can be added in the future if there is > a driver, which needs that. It is a design mistake to base new kernel interfaces on hardware quirks. > Is it worth posting an implementation in that direction? Sure, but please make thoughts about how this would work for the non-igc world. IIRC one of the nxp switches also has such counters? You can start with that. Thanks, Richard
> > For igc and tsnep the 16 bytes in front of the RX frame exist anyway. > > So this would be a minimal solution with best performance as a first > > step. A lookup for netdev/phc can be added in the future if there is > > a driver, which needs that. > > It is a design mistake to base new kernel interfaces on hardware > quirks. > > > Is it worth posting an implementation in that direction? > > Sure, but please make thoughts about how this would work for the > non-igc world. > > IIRC one of the nxp switches also has such counters? You can start > with that. I did some measurements (A53, 1.2GHz): netdev lookup and call to my driver takes ~400ns. ptp_convert_timestamp() takes ~6000ns, I assume because of class_find_device_by_name() call. So eliminating the netdev lookup is the wrong optimisation target. I will try to do it like that: - normal, driver without cycles support: use hwtstamps->hwtstamp directly (not changed) - driver with cycles support: netdev lookup for address/cookie conversion to hwtstamp (newly implemented) - vclock: slow path with phc lookup (not changed) Gerhard
On Sat, Apr 16, 2022 at 12:04:20AM +0200, Gerhard Engleder wrote:
> - vclock: slow path with phc lookup (not changed)
This one could really use caching. (I know that isn't your patch, but
maybe you can try to fix it?)
Thanks,
Richard
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 59e27a2b7bf0..f6cc4c673082 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1353,6 +1353,12 @@ struct netdev_net_notifier { * The caller must be under RCU read context. * int (*ndo_fill_forward_path)(struct net_device_path_ctx *ctx, struct net_device_path *path); * Get the forwarding path to reach the real device from the HW destination address + * ktime_t (*ndo_get_tstamp)(struct net_device *dev, + * const struct skb_shared_hwtstamps *hwtstamps, + * bool cycles); + * Get hardware timestamp based on normal/adjustable time or free running + * cycle counter. This function is required if physical clock supports a + * free running cycle counter. */ struct net_device_ops { int (*ndo_init)(struct net_device *dev); @@ -1570,6 +1576,9 @@ struct net_device_ops { struct net_device * (*ndo_get_peer_dev)(struct net_device *dev); int (*ndo_fill_forward_path)(struct net_device_path_ctx *ctx, struct net_device_path *path); + ktime_t (*ndo_get_tstamp)(struct net_device *dev, + const struct skb_shared_hwtstamps *hwtstamps, + bool cycles); }; /** @@ -4764,6 +4773,18 @@ static inline void netdev_rx_csum_fault(struct net_device *dev, void net_enable_timestamp(void); void net_disable_timestamp(void); +static inline ktime_t netdev_get_tstamp(struct net_device *dev, + const struct skb_shared_hwtstamps *hwtstamps, + bool cycles) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + if (ops->ndo_get_tstamp) + return ops->ndo_get_tstamp(dev, hwtstamps, cycles); + + return hwtstamps->hwtstamp; +} + #ifdef CONFIG_PROC_FS int __init dev_proc_init(void); #else diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index aeb3ed4d6cf8..c428b678e7f1 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -551,8 +551,10 @@ static inline bool skb_frag_must_loop(struct page *p) /** * struct skb_shared_hwtstamps - hardware time stamps - * @hwtstamp: hardware time stamp transformed into duration - * since arbitrary point in time + * @hwtstamp: hardware time stamp transformed into duration + * since arbitrary point in time + * @netdev_data: address/cookie of network device driver used as + * reference to actual hardware time stamp * * Software time stamps generated by ktime_get_real() are stored in * skb->tstamp. @@ -564,7 +566,10 @@ static inline bool skb_frag_must_loop(struct page *p) * &skb_shared_info. Use skb_hwtstamps() to get a pointer. */ struct skb_shared_hwtstamps { - ktime_t hwtstamp; + union { + ktime_t hwtstamp; + void *netdev_data; + }; }; /* Definitions for tx_flags in struct skb_shared_info */ diff --git a/net/socket.c b/net/socket.c index 4801aeaeb285..d64bd3dfcf6a 100644 --- a/net/socket.c +++ b/net/socket.c @@ -805,21 +805,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp) return skb->tstamp && !false_tstamp && skb_is_err_queue(skb); } -static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb) +static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb, + int if_index) { struct scm_ts_pktinfo ts_pktinfo; - struct net_device *orig_dev; if (!skb_mac_header_was_set(skb)) return; memset(&ts_pktinfo, 0, sizeof(ts_pktinfo)); - rcu_read_lock(); - orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); - if (orig_dev) - ts_pktinfo.if_index = orig_dev->ifindex; - rcu_read_unlock(); + ts_pktinfo.if_index = if_index; ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb); put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO, @@ -839,6 +835,8 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, int empty = 1, false_tstamp = 0; struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb); + struct net_device *orig_dev; + int if_index; ktime_t hwtstamp; /* Race occurred between timestamp enabling and packet @@ -887,18 +885,28 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, if (shhwtstamps && (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && !skb_is_swtx_tstamp(skb, false_tstamp)) { + rcu_read_lock(); + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); + if (orig_dev) { + if_index = orig_dev->ifindex; + hwtstamp = netdev_get_tstamp(orig_dev, shhwtstamps, + sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC); + } else { + if_index = 0; + hwtstamp = shhwtstamps->hwtstamp; + } + rcu_read_unlock(); + if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) - hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp, + hwtstamp = ptp_convert_timestamp(&hwtstamp, sk->sk_bind_phc); - else - hwtstamp = shhwtstamps->hwtstamp; if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) { empty = 0; if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) && !skb_is_err_queue(skb)) - put_ts_pktinfo(msg, skb); + put_ts_pktinfo(msg, skb, if_index); } } if (!empty) {
If a physical clock supports a free running cycle counter, then timestamps shall be based on this time too. For TX it is known in advance before the transmission if a timestamp based on the free running cycle counter is needed. For RX it is impossible to know which timestamp is needed before the packet is received and assigned to a socket. Support late timestamp determination by a network device. Therefore, an address/cookie is stored within the new netdev_data field of struct skb_shared_hwtstamps. This address/cookie is provided to a new network device function called ndo_get_tstamp(), which returns a timestamp based on the normal/adjustable time or based on the free running cycle counter. If function is not supported, then timestamp handling is not changed. Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> --- include/linux/netdevice.h | 21 +++++++++++++++++++++ include/linux/skbuff.h | 11 ++++++++--- net/socket.c | 30 +++++++++++++++++++----------- 3 files changed, 48 insertions(+), 14 deletions(-)