| Message ID | GtgHtyGO5jHKHT6zGMAzg3TDejXZT0HMQVoqNERZRdM@cp3-web-024.plabs.ch (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: allow virtual netdevs to forward UDP L4 and fraglist GSO skbs | expand |
On Sun, Nov 1, 2020 at 8:17 AM Alexander Lobakin <alobakin@pm.me> wrote: > > Virtual netdevs should use NETIF_F_GSO_SOFTWARE to forward GSO skbs > as-is and let the final drivers deal with them when supported. > Also remove NETIF_F_GSO_UDP_L4 from bonding and team drivers as it's > now included in the "software" list. The rationale is that it is okay to advertise these features with software fallback as bonding/teaming "hardware" features, because there will always be a downstream device for which they will be implemented, possibly in the software fallback, correct? That does not apply to dummy or IFB. I guess dummy is fine, because xmit is a black hole, and IFB because ingress can safely handle these packets? How did you arrive at the choice of changing these two, of all virtual devices? > > Suggested-by: Willem de Bruijn <willemb@google.com> > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > --- > drivers/net/bonding/bond_main.c | 11 +++++------ > drivers/net/dummy.c | 2 +- > drivers/net/ifb.c | 3 +-- > drivers/net/team/team.c | 9 ++++----- > 4 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 84ecbc6fa0ff..71c9677d135f 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1228,14 +1228,14 @@ static netdev_features_t bond_fix_features(struct net_device *dev, > } > > #define BOND_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ > - NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \ > + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \ > NETIF_F_HIGHDMA | NETIF_F_LRO) > > #define BOND_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ > - NETIF_F_RXCSUM | NETIF_F_ALL_TSO) > + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE) > > #define BOND_MPLS_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ > - NETIF_F_ALL_TSO) > + NETIF_F_GSO_SOFTWARE) > > > static void bond_compute_features(struct bonding *bond) > @@ -1291,8 +1291,7 @@ static void bond_compute_features(struct bonding *bond) > bond_dev->vlan_features = vlan_features; > bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | > NETIF_F_HW_VLAN_CTAG_TX | > - NETIF_F_HW_VLAN_STAG_TX | > - NETIF_F_GSO_UDP_L4; > + NETIF_F_HW_VLAN_STAG_TX; > #ifdef CONFIG_XFRM_OFFLOAD > bond_dev->hw_enc_features |= xfrm_features; > #endif /* CONFIG_XFRM_OFFLOAD */ > @@ -4721,7 +4720,7 @@ void bond_setup(struct net_device *bond_dev) > NETIF_F_HW_VLAN_CTAG_RX | > NETIF_F_HW_VLAN_CTAG_FILTER; > > - bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4; > + bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL; > #ifdef CONFIG_XFRM_OFFLOAD > bond_dev->hw_features |= BOND_XFRM_FEATURES; > #endif /* CONFIG_XFRM_OFFLOAD */ > diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c > index bab3a9bb5e6f..f82ad7419508 100644 > --- a/drivers/net/dummy.c > +++ b/drivers/net/dummy.c > @@ -124,7 +124,7 @@ static void dummy_setup(struct net_device *dev) > dev->flags &= ~IFF_MULTICAST; > dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE; > dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST; > - dev->features |= NETIF_F_ALL_TSO; > + dev->features |= NETIF_F_GSO_SOFTWARE; > dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; > dev->features |= NETIF_F_GSO_ENCAP_ALL; > dev->hw_features |= dev->features; > diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c > index 7fe306e76281..fa63d4dee0ba 100644 > --- a/drivers/net/ifb.c > +++ b/drivers/net/ifb.c > @@ -187,8 +187,7 @@ static const struct net_device_ops ifb_netdev_ops = { > }; > > #define IFB_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_FRAGLIST | \ > - NETIF_F_TSO_ECN | NETIF_F_TSO | NETIF_F_TSO6 | \ > - NETIF_F_GSO_ENCAP_ALL | \ > + NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ENCAP_ALL | \ > NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_CTAG_TX | \ > NETIF_F_HW_VLAN_STAG_TX) > > diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c > index 07f1f3933927..b4092127a92c 100644 > --- a/drivers/net/team/team.c > +++ b/drivers/net/team/team.c > @@ -975,11 +975,11 @@ static void team_port_disable(struct team *team, > } > > #define TEAM_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ > - NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \ > + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \ > NETIF_F_HIGHDMA | NETIF_F_LRO) > > #define TEAM_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ > - NETIF_F_RXCSUM | NETIF_F_ALL_TSO) > + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE) > > static void __team_compute_features(struct team *team) > { > @@ -1009,8 +1009,7 @@ static void __team_compute_features(struct team *team) > team->dev->vlan_features = vlan_features; > team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | > NETIF_F_HW_VLAN_CTAG_TX | > - NETIF_F_HW_VLAN_STAG_TX | > - NETIF_F_GSO_UDP_L4; > + NETIF_F_HW_VLAN_STAG_TX; > team->dev->hard_header_len = max_hard_header_len; > > team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE; > @@ -2175,7 +2174,7 @@ static void team_setup(struct net_device *dev) > NETIF_F_HW_VLAN_CTAG_RX | > NETIF_F_HW_VLAN_CTAG_FILTER; > > - dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4; > + dev->hw_features |= NETIF_F_GSO_ENCAP_ALL; > dev->features |= dev->hw_features; > dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX; > } > -- > 2.29.2 > >
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: Mon, 2 Nov 2020 11:30:17 -0500 Hi! Thanks for the Ack. > On Sun, Nov 1, 2020 at 8:17 AM Alexander Lobakin <alobakin@pm.me> wrote: >> >> Virtual netdevs should use NETIF_F_GSO_SOFTWARE to forward GSO skbs >> as-is and let the final drivers deal with them when supported. >> Also remove NETIF_F_GSO_UDP_L4 from bonding and team drivers as it's >> now included in the "software" list. > > The rationale is that it is okay to advertise these features with > software fallback as bonding/teaming "hardware" features, because > there will always be a downstream device for which they will be > implemented, possibly in the software fallback, correct? > > That does not apply to dummy or IFB. I guess dummy is fine, because > xmit is a black hole, and IFB because ingress can safely handle these > packets? How did you arrive at the choice of changing these two, of > all virtual devices? Two points: 1. Exactly, dummy is just dummy, while ifb is an intermediate netdev to share resources, so it should be as fine as with other virtual devs. 2. They both advertise NETIF_F_ALL_TSO | NETIF_F_GSO_ENCAP_ALL, which assumes that they handle all GSO skbs just like the others (pass them as is to the real drivers in case with ifb). >> >> Suggested-by: Willem de Bruijn <willemb@google.com> >> Signed-off-by: Alexander Lobakin <alobakin@pm.me> >> --- >> drivers/net/bonding/bond_main.c | 11 +++++------ >> drivers/net/dummy.c | 2 +- >> drivers/net/ifb.c | 3 +-- >> drivers/net/team/team.c | 9 ++++----- >> 4 files changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 84ecbc6fa0ff..71c9677d135f 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1228,14 +1228,14 @@ static netdev_features_t bond_fix_features(struct net_device *dev, >> } >> >> #define BOND_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ >> - NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \ >> + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \ >> NETIF_F_HIGHDMA | NETIF_F_LRO) >> >> #define BOND_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ >> - NETIF_F_RXCSUM | NETIF_F_ALL_TSO) >> + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE) >> >> #define BOND_MPLS_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ >> - NETIF_F_ALL_TSO) >> + NETIF_F_GSO_SOFTWARE) >> >> >> static void bond_compute_features(struct bonding *bond) >> @@ -1291,8 +1291,7 @@ static void bond_compute_features(struct bonding *bond) >> bond_dev->vlan_features = vlan_features; >> bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | >> NETIF_F_HW_VLAN_CTAG_TX | >> - NETIF_F_HW_VLAN_STAG_TX | >> - NETIF_F_GSO_UDP_L4; >> + NETIF_F_HW_VLAN_STAG_TX; >> #ifdef CONFIG_XFRM_OFFLOAD >> bond_dev->hw_enc_features |= xfrm_features; >> #endif /* CONFIG_XFRM_OFFLOAD */ >> @@ -4721,7 +4720,7 @@ void bond_setup(struct net_device *bond_dev) >> NETIF_F_HW_VLAN_CTAG_RX | >> NETIF_F_HW_VLAN_CTAG_FILTER; >> >> - bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4; >> + bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL; >> #ifdef CONFIG_XFRM_OFFLOAD >> bond_dev->hw_features |= BOND_XFRM_FEATURES; >> #endif /* CONFIG_XFRM_OFFLOAD */ >> diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c >> index bab3a9bb5e6f..f82ad7419508 100644 >> --- a/drivers/net/dummy.c >> +++ b/drivers/net/dummy.c >> @@ -124,7 +124,7 @@ static void dummy_setup(struct net_device *dev) >> dev->flags &= ~IFF_MULTICAST; >> dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE; >> dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST; >> - dev->features |= NETIF_F_ALL_TSO; >> + dev->features |= NETIF_F_GSO_SOFTWARE; >> dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; >> dev->features |= NETIF_F_GSO_ENCAP_ALL; >> dev->hw_features |= dev->features; >> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c >> index 7fe306e76281..fa63d4dee0ba 100644 >> --- a/drivers/net/ifb.c >> +++ b/drivers/net/ifb.c >> @@ -187,8 +187,7 @@ static const struct net_device_ops ifb_netdev_ops = { >> }; >> >> #define IFB_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_FRAGLIST | \ >> - NETIF_F_TSO_ECN | NETIF_F_TSO | NETIF_F_TSO6 | \ >> - NETIF_F_GSO_ENCAP_ALL | \ >> + NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ENCAP_ALL | \ >> NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_CTAG_TX | \ >> NETIF_F_HW_VLAN_STAG_TX) >> >> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >> index 07f1f3933927..b4092127a92c 100644 >> --- a/drivers/net/team/team.c >> +++ b/drivers/net/team/team.c >> @@ -975,11 +975,11 @@ static void team_port_disable(struct team *team, >> } >> >> #define TEAM_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ >> - NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \ >> + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \ >> NETIF_F_HIGHDMA | NETIF_F_LRO) >> >> #define TEAM_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ >> - NETIF_F_RXCSUM | NETIF_F_ALL_TSO) >> + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE) >> >> static void __team_compute_features(struct team *team) >> { >> @@ -1009,8 +1009,7 @@ static void __team_compute_features(struct team *team) >> team->dev->vlan_features = vlan_features; >> team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | >> NETIF_F_HW_VLAN_CTAG_TX | >> - NETIF_F_HW_VLAN_STAG_TX | >> - NETIF_F_GSO_UDP_L4; >> + NETIF_F_HW_VLAN_STAG_TX; >> team->dev->hard_header_len = max_hard_header_len; >> >> team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE; >> @@ -2175,7 +2174,7 @@ static void team_setup(struct net_device *dev) >> NETIF_F_HW_VLAN_CTAG_RX | >> NETIF_F_HW_VLAN_CTAG_FILTER; >> >> - dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4; >> + dev->hw_features |= NETIF_F_GSO_ENCAP_ALL; >> dev->features |= dev->hw_features; >> dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX; >> } >> -- >> 2.29.2 Al
On Mon, Nov 2, 2020 at 2:26 PM Alexander Lobakin <alobakin@pm.me> wrote: > > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Date: Mon, 2 Nov 2020 11:30:17 -0500 > > Hi! > Thanks for the Ack. > > > On Sun, Nov 1, 2020 at 8:17 AM Alexander Lobakin <alobakin@pm.me> wrote: > >> > >> Virtual netdevs should use NETIF_F_GSO_SOFTWARE to forward GSO skbs > >> as-is and let the final drivers deal with them when supported. > >> Also remove NETIF_F_GSO_UDP_L4 from bonding and team drivers as it's > >> now included in the "software" list. > > > > The rationale is that it is okay to advertise these features with > > software fallback as bonding/teaming "hardware" features, because > > there will always be a downstream device for which they will be > > implemented, possibly in the software fallback, correct? > > > > That does not apply to dummy or IFB. I guess dummy is fine, because > > xmit is a black hole, and IFB because ingress can safely handle these > > packets? How did you arrive at the choice of changing these two, of > > all virtual devices? > > Two points: > 1. Exactly, dummy is just dummy, while ifb is an intermediate netdev to > share resources, so it should be as fine as with other virtual devs. > 2. They both advertise NETIF_F_ALL_TSO | NETIF_F_GSO_ENCAP_ALL, which > assumes that they handle all GSO skbs just like the others (pass > them as is to the real drivers in case with ifb). There is no real driver in the case of ifb if it forwards to the ingress path. But as discussed before, that can handle gso packets for all these protocols, too. > >> > >> Suggested-by: Willem de Bruijn <willemb@google.com> > >> Signed-off-by: Alexander Lobakin <alobakin@pm.me> Acked-by: Willem de Bruijn <willemb@google.com>
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 84ecbc6fa0ff..71c9677d135f 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1228,14 +1228,14 @@ static netdev_features_t bond_fix_features(struct net_device *dev, } #define BOND_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ - NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \ + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \ NETIF_F_HIGHDMA | NETIF_F_LRO) #define BOND_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ - NETIF_F_RXCSUM | NETIF_F_ALL_TSO) + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE) #define BOND_MPLS_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ - NETIF_F_ALL_TSO) + NETIF_F_GSO_SOFTWARE) static void bond_compute_features(struct bonding *bond) @@ -1291,8 +1291,7 @@ static void bond_compute_features(struct bonding *bond) bond_dev->vlan_features = vlan_features; bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | NETIF_F_HW_VLAN_CTAG_TX | - NETIF_F_HW_VLAN_STAG_TX | - NETIF_F_GSO_UDP_L4; + NETIF_F_HW_VLAN_STAG_TX; #ifdef CONFIG_XFRM_OFFLOAD bond_dev->hw_enc_features |= xfrm_features; #endif /* CONFIG_XFRM_OFFLOAD */ @@ -4721,7 +4720,7 @@ void bond_setup(struct net_device *bond_dev) NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER; - bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4; + bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL; #ifdef CONFIG_XFRM_OFFLOAD bond_dev->hw_features |= BOND_XFRM_FEATURES; #endif /* CONFIG_XFRM_OFFLOAD */ diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c index bab3a9bb5e6f..f82ad7419508 100644 --- a/drivers/net/dummy.c +++ b/drivers/net/dummy.c @@ -124,7 +124,7 @@ static void dummy_setup(struct net_device *dev) dev->flags &= ~IFF_MULTICAST; dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE; dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST; - dev->features |= NETIF_F_ALL_TSO; + dev->features |= NETIF_F_GSO_SOFTWARE; dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX; dev->features |= NETIF_F_GSO_ENCAP_ALL; dev->hw_features |= dev->features; diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c index 7fe306e76281..fa63d4dee0ba 100644 --- a/drivers/net/ifb.c +++ b/drivers/net/ifb.c @@ -187,8 +187,7 @@ static const struct net_device_ops ifb_netdev_ops = { }; #define IFB_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | NETIF_F_FRAGLIST | \ - NETIF_F_TSO_ECN | NETIF_F_TSO | NETIF_F_TSO6 | \ - NETIF_F_GSO_ENCAP_ALL | \ + NETIF_F_GSO_SOFTWARE | NETIF_F_GSO_ENCAP_ALL | \ NETIF_F_HIGHDMA | NETIF_F_HW_VLAN_CTAG_TX | \ NETIF_F_HW_VLAN_STAG_TX) diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c index 07f1f3933927..b4092127a92c 100644 --- a/drivers/net/team/team.c +++ b/drivers/net/team/team.c @@ -975,11 +975,11 @@ static void team_port_disable(struct team *team, } #define TEAM_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ - NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \ + NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE | \ NETIF_F_HIGHDMA | NETIF_F_LRO) #define TEAM_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \ - NETIF_F_RXCSUM | NETIF_F_ALL_TSO) + NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE) static void __team_compute_features(struct team *team) { @@ -1009,8 +1009,7 @@ static void __team_compute_features(struct team *team) team->dev->vlan_features = vlan_features; team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | NETIF_F_HW_VLAN_CTAG_TX | - NETIF_F_HW_VLAN_STAG_TX | - NETIF_F_GSO_UDP_L4; + NETIF_F_HW_VLAN_STAG_TX; team->dev->hard_header_len = max_hard_header_len; team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE; @@ -2175,7 +2174,7 @@ static void team_setup(struct net_device *dev) NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_FILTER; - dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4; + dev->hw_features |= NETIF_F_GSO_ENCAP_ALL; dev->features |= dev->hw_features; dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX; }
Virtual netdevs should use NETIF_F_GSO_SOFTWARE to forward GSO skbs as-is and let the final drivers deal with them when supported. Also remove NETIF_F_GSO_UDP_L4 from bonding and team drivers as it's now included in the "software" list. Suggested-by: Willem de Bruijn <willemb@google.com> Signed-off-by: Alexander Lobakin <alobakin@pm.me> --- drivers/net/bonding/bond_main.c | 11 +++++------ drivers/net/dummy.c | 2 +- drivers/net/ifb.c | 3 +-- drivers/net/team/team.c | 9 ++++----- 4 files changed, 11 insertions(+), 14 deletions(-)