| Message ID | 20210315135545.737069480@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Mar 15, 2021 at 07:56:02PM +0000, Vladimir Oltean wrote: > +Andrew, Vivien, > > On Mon, Mar 15, 2021 at 02:53:26PM +0100, gregkh@linuxfoundation.org wrote: > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > [ Upstream commit a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2 ] > > > > At the moment, taggers are left with the task of ensuring that the skb > > headers are writable (which they aren't, if the frames were cloned for > > TX timestamping, for flooding by the bridge, etc), and that there is > > enough space in the skb data area for the DSA tag to be pushed. > > > > Moreover, the life of tail taggers is even harder, because they need to > > ensure that short frames have enough padding, a problem that normal > > taggers don't have. > > > > The principle of the DSA framework is that everything except for the > > most intimate hardware specifics (like in this case, the actual packing > > of the DSA tag bits) should be done inside the core, to avoid having > > code paths that are very rarely tested. > > > > So provide a TX reallocation procedure that should cover the known needs > > of DSA today. > > > > Note that this patch also gives the network stack a good hint about the > > headroom/tailroom it's going to need. Up till now it wasn't doing that. > > So the reallocation procedure should really be there only for the > > exceptional cases, and for cloned packets which need to be unshared. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Tested-by: Christian Eggers <ceggers@arri.de> # For tail taggers only > > Tested-by: Kurt Kanzenbach <kurt@linutronix.de> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > Signed-off-by: Sasha Levin <sashal@kernel.org> > > --- > > For context, Sasha explains here: > https://www.spinics.net/lists/stable-commits/msg190151.html > (the conversation is somewhat truncated, unfortunately, because > stable-commits@vger.kernel.org ate my replies) > that 13 patches were backported to get the unrelated commit 9200f515c41f > ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am. > > I am not strictly against this, even though I would have liked to know > that the maintainers were explicitly informed about it. > > Greg, could you make your stable backporting emails include the output > of ./get_maintainer.pl into the list of recipients? Thanks. I cc: everyone on the signed-off-by list on the patch, why would we need to add more? A maintainer should always be on that list automatically. thanks, greg k-h
On Mon, Mar 15, 2021 at 05:06:16PM -0400, Sasha Levin wrote: > On Mon, Mar 15, 2021 at 07:56:02PM +0000, Vladimir Oltean wrote: > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > Tested-by: Christian Eggers <ceggers@arri.de> # For tail taggers only > > > Tested-by: Kurt Kanzenbach <kurt@linutronix.de> > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > > Signed-off-by: Sasha Levin <sashal@kernel.org> > > > --- > > > > For context, Sasha explains here: > > https://www.spinics.net/lists/stable-commits/msg190151.html > > (the conversation is somewhat truncated, unfortunately, because > > stable-commits@vger.kernel.org ate my replies) > > that 13 patches were backported to get the unrelated commit 9200f515c41f > > ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am. > > > > I am not strictly against this, even though I would have liked to know > > that the maintainers were explicitly informed about it. > > > > Greg, could you make your stable backporting emails include the output > > of ./get_maintainer.pl into the list of recipients? Thanks. > > Did it not happen here? I've looked at Greg's script[1] and it seemed to > me like it does go through get_maintainer.pl. > > > [1] https://github.com/gregkh/gregkh-linux/blob/master/scripts/generate_cc_list That's just a script I use for "normal" kernel development when creating patches, not for stable stuff. thanks, greg k-h
On Tue, Mar 16, 2021 at 06:46:10AM +0100, gregkh@linuxfoundation.org wrote: >On Mon, Mar 15, 2021 at 07:56:02PM +0000, Vladimir Oltean wrote: >> +Andrew, Vivien, >> >> On Mon, Mar 15, 2021 at 02:53:26PM +0100, gregkh@linuxfoundation.org wrote: >> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> > >> > From: Vladimir Oltean <vladimir.oltean@nxp.com> >> > >> > [ Upstream commit a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2 ] >> > >> > At the moment, taggers are left with the task of ensuring that the skb >> > headers are writable (which they aren't, if the frames were cloned for >> > TX timestamping, for flooding by the bridge, etc), and that there is >> > enough space in the skb data area for the DSA tag to be pushed. >> > >> > Moreover, the life of tail taggers is even harder, because they need to >> > ensure that short frames have enough padding, a problem that normal >> > taggers don't have. >> > >> > The principle of the DSA framework is that everything except for the >> > most intimate hardware specifics (like in this case, the actual packing >> > of the DSA tag bits) should be done inside the core, to avoid having >> > code paths that are very rarely tested. >> > >> > So provide a TX reallocation procedure that should cover the known needs >> > of DSA today. >> > >> > Note that this patch also gives the network stack a good hint about the >> > headroom/tailroom it's going to need. Up till now it wasn't doing that. >> > So the reallocation procedure should really be there only for the >> > exceptional cases, and for cloned packets which need to be unshared. >> > >> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >> > Tested-by: Christian Eggers <ceggers@arri.de> # For tail taggers only >> > Tested-by: Kurt Kanzenbach <kurt@linutronix.de> >> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> >> > Signed-off-by: Sasha Levin <sashal@kernel.org> >> > --- >> >> For context, Sasha explains here: >> https://www.spinics.net/lists/stable-commits/msg190151.html >> (the conversation is somewhat truncated, unfortunately, because >> stable-commits@vger.kernel.org ate my replies) >> that 13 patches were backported to get the unrelated commit 9200f515c41f >> ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am. >> >> I am not strictly against this, even though I would have liked to know >> that the maintainers were explicitly informed about it. >> >> Greg, could you make your stable backporting emails include the output >> of ./get_maintainer.pl into the list of recipients? Thanks. > >I cc: everyone on the signed-off-by list on the patch, why would we need >to add more? A maintainer should always be on that list automatically. Oh, hm, could this be an issue with subsystems that have a shared maintainership model? In that scenario not all maintainers will sign-off on a commit. -- Thanks, Sasha
On Tue, Mar 16, 2021 at 09:54:01AM -0400, Sasha Levin wrote: > On Tue, Mar 16, 2021 at 06:46:10AM +0100, gregkh@linuxfoundation.org wrote: > > On Mon, Mar 15, 2021 at 07:56:02PM +0000, Vladimir Oltean wrote: > > > +Andrew, Vivien, > > > > > > On Mon, Mar 15, 2021 at 02:53:26PM +0100, gregkh@linuxfoundation.org wrote: > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > > > > > [ Upstream commit a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2 ] > > > > > > > > At the moment, taggers are left with the task of ensuring that the skb > > > > headers are writable (which they aren't, if the frames were cloned for > > > > TX timestamping, for flooding by the bridge, etc), and that there is > > > > enough space in the skb data area for the DSA tag to be pushed. > > > > > > > > Moreover, the life of tail taggers is even harder, because they need to > > > > ensure that short frames have enough padding, a problem that normal > > > > taggers don't have. > > > > > > > > The principle of the DSA framework is that everything except for the > > > > most intimate hardware specifics (like in this case, the actual packing > > > > of the DSA tag bits) should be done inside the core, to avoid having > > > > code paths that are very rarely tested. > > > > > > > > So provide a TX reallocation procedure that should cover the known needs > > > > of DSA today. > > > > > > > > Note that this patch also gives the network stack a good hint about the > > > > headroom/tailroom it's going to need. Up till now it wasn't doing that. > > > > So the reallocation procedure should really be there only for the > > > > exceptional cases, and for cloned packets which need to be unshared. > > > > > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Tested-by: Christian Eggers <ceggers@arri.de> # For tail taggers only > > > > Tested-by: Kurt Kanzenbach <kurt@linutronix.de> > > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > > > Signed-off-by: Sasha Levin <sashal@kernel.org> > > > > --- > > > > > > For context, Sasha explains here: > > > https://www.spinics.net/lists/stable-commits/msg190151.html > > > (the conversation is somewhat truncated, unfortunately, because > > > stable-commits@vger.kernel.org ate my replies) > > > that 13 patches were backported to get the unrelated commit 9200f515c41f > > > ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am. > > > > > > I am not strictly against this, even though I would have liked to know > > > that the maintainers were explicitly informed about it. > > > > > > Greg, could you make your stable backporting emails include the output > > > of ./get_maintainer.pl into the list of recipients? Thanks. > > > > I cc: everyone on the signed-off-by list on the patch, why would we need > > to add more? A maintainer should always be on that list automatically. > > Oh, hm, could this be an issue with subsystems that have a shared > maintainership model? In that scenario not all maintainers will sign-off > on a commit. So a shared maintainer trusts their co-maintainer for reviewing patches for Linus's tree and all future kernels, but NOT into an old backported stable tree? I doubt that, trust should be the same for both. thanks, greg k-h
On Tue, Mar 16, 2021 at 05:05:11PM +0100, gregkh@linuxfoundation.org wrote: >On Tue, Mar 16, 2021 at 09:54:01AM -0400, Sasha Levin wrote: >> On Tue, Mar 16, 2021 at 06:46:10AM +0100, gregkh@linuxfoundation.org wrote: >> > I cc: everyone on the signed-off-by list on the patch, why would we need >> > to add more? A maintainer should always be on that list automatically. >> >> Oh, hm, could this be an issue with subsystems that have a shared >> maintainership model? In that scenario not all maintainers will sign-off >> on a commit. > >So a shared maintainer trusts their co-maintainer for reviewing patches >for Linus's tree and all future kernels, but NOT into an old backported >stable tree? I doubt that, trust should be the same for both. I don't think it's necessarily a trust issue, but is an availability issue: one of the reasons shared maintainership models exist is so that one maintainer can go on vacation (or focus other work) while the other maintainer(s) take over. If we send a review request to that maintainer he might be away and we'll never get our review. -- Thanks, Sasha
On Tue, Mar 16, 2021 at 05:05:11PM +0100, gregkh@linuxfoundation.org wrote: > On Tue, Mar 16, 2021 at 09:54:01AM -0400, Sasha Levin wrote: > > On Tue, Mar 16, 2021 at 06:46:10AM +0100, gregkh@linuxfoundation.org wrote: > > > On Mon, Mar 15, 2021 at 07:56:02PM +0000, Vladimir Oltean wrote: > > > > +Andrew, Vivien, > > > > > > > > On Mon, Mar 15, 2021 at 02:53:26PM +0100, gregkh@linuxfoundation.org wrote: > > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > > > > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > > > > > > > [ Upstream commit a3b0b6479700a5b0af2c631cb2ec0fb7a0d978f2 ] > > > > > > > > > > At the moment, taggers are left with the task of ensuring that the skb > > > > > headers are writable (which they aren't, if the frames were cloned for > > > > > TX timestamping, for flooding by the bridge, etc), and that there is > > > > > enough space in the skb data area for the DSA tag to be pushed. > > > > > > > > > > Moreover, the life of tail taggers is even harder, because they need to > > > > > ensure that short frames have enough padding, a problem that normal > > > > > taggers don't have. > > > > > > > > > > The principle of the DSA framework is that everything except for the > > > > > most intimate hardware specifics (like in this case, the actual packing > > > > > of the DSA tag bits) should be done inside the core, to avoid having > > > > > code paths that are very rarely tested. > > > > > > > > > > So provide a TX reallocation procedure that should cover the known needs > > > > > of DSA today. > > > > > > > > > > Note that this patch also gives the network stack a good hint about the > > > > > headroom/tailroom it's going to need. Up till now it wasn't doing that. > > > > > So the reallocation procedure should really be there only for the > > > > > exceptional cases, and for cloned packets which need to be unshared. > > > > > > > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > > Tested-by: Christian Eggers <ceggers@arri.de> # For tail taggers only > > > > > Tested-by: Kurt Kanzenbach <kurt@linutronix.de> > > > > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > > > > Signed-off-by: Sasha Levin <sashal@kernel.org> > > > > > --- > > > > > > > > For context, Sasha explains here: > > > > https://www.spinics.net/lists/stable-commits/msg190151.html > > > > (the conversation is somewhat truncated, unfortunately, because > > > > stable-commits@vger.kernel.org ate my replies) > > > > that 13 patches were backported to get the unrelated commit 9200f515c41f > > > > ("net: dsa: tag_mtk: fix 802.1ad VLAN egress") to apply cleanly with git-am. > > > > > > > > I am not strictly against this, even though I would have liked to know > > > > that the maintainers were explicitly informed about it. > > > > > > > > Greg, could you make your stable backporting emails include the output > > > > of ./get_maintainer.pl into the list of recipients? Thanks. > > > > > > I cc: everyone on the signed-off-by list on the patch, why would we need > > > to add more? A maintainer should always be on that list automatically. > > > > Oh, hm, could this be an issue with subsystems that have a shared > > maintainership model? In that scenario not all maintainers will sign-off > > on a commit. > > So a shared maintainer trusts their co-maintainer for reviewing patches > for Linus's tree and all future kernels, but NOT into an old backported > stable tree? I doubt that, trust should be the same for both. Greg, the problem is that we have the following maintainership layout: General networking maintainers (David Miller && Jakub Kicinski) -> DSA framework maintainers -> DSA hardware driver maintainers But there is a single tree with mandatory sign-off from a single maintainer, and that would be David or Jakub. And in rare cases it may happen for patches to get accepted without the written ACK of any of the sub-maintainers. If the question is whether I trust David or Jakub to pay attention on why are 13 patches that don't fix anything being backported to stable, and then take the time to check/test whether anything is going to be broken in subtle ways because code was backported in places it was never meant to belong, then yeah, sorry, but no. In this case, things could have gone a lot worse: the model you're following makes it possible to backport a breaking change into a subsystem and the maintainer can never find out until there'a a bug report.
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 3bc5ca40c9fb..c6806eef906f 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -548,6 +548,30 @@ netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev) } EXPORT_SYMBOL_GPL(dsa_enqueue_skb); +static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev) +{ + int needed_headroom = dev->needed_headroom; + int needed_tailroom = dev->needed_tailroom; + + /* For tail taggers, we need to pad short frames ourselves, to ensure + * that the tail tag does not fail at its role of being at the end of + * the packet, once the master interface pads the frame. Account for + * that pad length here, and pad later. + */ + if (unlikely(needed_tailroom && skb->len < ETH_ZLEN)) + needed_tailroom += ETH_ZLEN - skb->len; + /* skb_headroom() returns unsigned int... */ + needed_headroom = max_t(int, needed_headroom - skb_headroom(skb), 0); + needed_tailroom = max_t(int, needed_tailroom - skb_tailroom(skb), 0); + + if (likely(!needed_headroom && !needed_tailroom && !skb_cloned(skb))) + /* No reallocation needed, yay! */ + return 0; + + return pskb_expand_head(skb, needed_headroom, needed_tailroom, + GFP_ATOMIC); +} + static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev) { struct dsa_slave_priv *p = netdev_priv(dev); @@ -567,6 +591,17 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev) */ dsa_skb_tx_timestamp(p, skb); + if (dsa_realloc_skb(skb, dev)) { + dev_kfree_skb_any(skb); + return NETDEV_TX_OK; + } + + /* needed_tailroom should still be 'warm' in the cache line from + * dsa_realloc_skb(), which has also ensured that padding is safe. + */ + if (dev->needed_tailroom) + eth_skb_pad(skb); + /* Transmit function may have to reallocate the original SKB, * in which case it must have freed it. Only free it here on error. */ @@ -1791,6 +1826,16 @@ int dsa_slave_create(struct dsa_port *port) slave_dev->netdev_ops = &dsa_slave_netdev_ops; if (ds->ops->port_max_mtu) slave_dev->max_mtu = ds->ops->port_max_mtu(ds, port->index); + if (cpu_dp->tag_ops->tail_tag) + slave_dev->needed_tailroom = cpu_dp->tag_ops->overhead; + else + slave_dev->needed_headroom = cpu_dp->tag_ops->overhead; + /* Try to save one extra realloc later in the TX path (in the master) + * by also inheriting the master's needed headroom and tailroom. + * The 8021q driver also does this. + */ + slave_dev->needed_headroom += master->needed_headroom; + slave_dev->needed_tailroom += master->needed_tailroom; SET_NETDEV_DEVTYPE(slave_dev, &dsa_type); netdev_for_each_tx_queue(slave_dev, dsa_slave_set_lockdep_class_one,