* [PATCH net-next 0/2] net: stmmac: Improvements for multi-queuing and for AVB
From: Jose Abreu @ 2017-10-12 15:14 UTC (permalink / raw)
To: netdev
Cc: Jose Abreu, David S. Miller, Joao Pinto, Giuseppe Cavallaro,
Alexandre Torgue
Hi,
Two improvements for stmmac: First one corrects the available fifo size per queue, second one corrects enabling of AVB queues. More info in commit log.
Best regards,
Jose Miguel Abreu
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Jose Abreu (2):
net: stmmac: Use correct values in TQS/RQS fields
net: stmmac: Disable flow ctrl for RX AVB queues and really enable TX
AVB queues
drivers/net/ethernet/stmicro/stmmac/common.h | 5 +--
drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 2 ++
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 27 ++++++++++------
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 39 +++++++++++++++++++----
4 files changed, 56 insertions(+), 17 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH net-next 1/2] net: stmmac: Use correct values in TQS/RQS fields
From: Jose Abreu @ 2017-10-12 15:14 UTC (permalink / raw)
To: netdev
Cc: Jose Abreu, David S. Miller, Joao Pinto, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <cover.1507820933.git.joabreu@synopsys.com>
Currently we are using all the available fifo size in RQS and
TQS fields. This will not work correctly in multi-queues IP's
because total fifo size must be splitted to the enabled queues.
Correct this by computing the available fifo size per queue and
setting the right value in TQS and RQS fields.
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
---
drivers/net/ethernet/stmicro/stmmac/common.h | 3 ++-
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 15 +++++++++------
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 22 ++++++++++++++++++++--
3 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index e82b4b7..c26c8a7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -443,7 +443,8 @@ struct stmmac_dma_ops {
int rxfifosz);
void (*dma_rx_mode)(void __iomem *ioaddr, int mode, u32 channel,
int fifosz);
- void (*dma_tx_mode)(void __iomem *ioaddr, int mode, u32 channel);
+ void (*dma_tx_mode)(void __iomem *ioaddr, int mode, u32 channel,
+ int fifosz);
/* To track extra statistic (if supported) */
void (*dma_diagnostic_fr) (void *data, struct stmmac_extra_stats *x,
void __iomem *ioaddr);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index e84831e..898849b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -271,9 +271,10 @@ static void dwmac4_dma_rx_chan_op_mode(void __iomem *ioaddr, int mode,
}
static void dwmac4_dma_tx_chan_op_mode(void __iomem *ioaddr, int mode,
- u32 channel)
+ u32 channel, int fifosz)
{
u32 mtl_tx_op = readl(ioaddr + MTL_CHAN_TX_OP_MODE(channel));
+ unsigned int tqs = fifosz / 256 - 1;
if (mode == SF_DMA_MODE) {
pr_debug("GMAC: enable TX store and forward mode\n");
@@ -306,12 +307,14 @@ static void dwmac4_dma_tx_chan_op_mode(void __iomem *ioaddr, int mode,
* For an IP with DWC_EQOS_NUM_TXQ > 1, the fields TXQEN and TQS are R/W
* with reset values: TXQEN off, TQS 256 bytes.
*
- * Write the bits in both cases, since it will have no effect when RO.
- * For DWC_EQOS_NUM_TXQ > 1, the top bits in MTL_OP_MODE_TQS_MASK might
- * be RO, however, writing the whole TQS field will result in a value
- * equal to DWC_EQOS_TXFIFO_SIZE, just like for DWC_EQOS_NUM_TXQ == 1.
+ * TXQEN must be written for multi-channel operation and TQS must
+ * reflect the available fifo size per queue (total fifo size / number
+ * of enabled queues).
*/
- mtl_tx_op |= MTL_OP_MODE_TXQEN | MTL_OP_MODE_TQS_MASK;
+ mtl_tx_op |= MTL_OP_MODE_TXQEN;
+ mtl_tx_op &= ~MTL_OP_MODE_TQS_MASK;
+ mtl_tx_op |= tqs << MTL_OP_MODE_TQS_SHIFT;
+
writel(mtl_tx_op, ioaddr + MTL_CHAN_TX_OP_MODE(channel));
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f41661a..edf245b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1750,12 +1750,19 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
u32 rx_channels_count = priv->plat->rx_queues_to_use;
u32 tx_channels_count = priv->plat->tx_queues_to_use;
int rxfifosz = priv->plat->rx_fifo_size;
+ int txfifosz = priv->plat->tx_fifo_size;
u32 txmode = 0;
u32 rxmode = 0;
u32 chan = 0;
if (rxfifosz == 0)
rxfifosz = priv->dma_cap.rx_fifo_size;
+ if (txfifosz == 0)
+ txfifosz = priv->dma_cap.tx_fifo_size;
+
+ /* Adjust for real per queue fifo size */
+ rxfifosz /= rx_channels_count;
+ txfifosz /= tx_channels_count;
if (priv->plat->force_thresh_dma_mode) {
txmode = tc;
@@ -1783,7 +1790,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
rxfifosz);
for (chan = 0; chan < tx_channels_count; chan++)
- priv->hw->dma->dma_tx_mode(priv->ioaddr, txmode, chan);
+ priv->hw->dma->dma_tx_mode(priv->ioaddr, txmode, chan,
+ txfifosz);
} else {
priv->hw->dma->dma_mode(priv->ioaddr, txmode, rxmode,
rxfifosz);
@@ -1946,15 +1954,25 @@ static void stmmac_tx_err(struct stmmac_priv *priv, u32 chan)
static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
u32 rxmode, u32 chan)
{
+ u32 rx_channels_count = priv->plat->rx_queues_to_use;
+ u32 tx_channels_count = priv->plat->tx_queues_to_use;
int rxfifosz = priv->plat->rx_fifo_size;
+ int txfifosz = priv->plat->tx_fifo_size;
if (rxfifosz == 0)
rxfifosz = priv->dma_cap.rx_fifo_size;
+ if (txfifosz == 0)
+ txfifosz = priv->dma_cap.tx_fifo_size;
+
+ /* Adjust for real per queue fifo size */
+ rxfifosz /= rx_channels_count;
+ txfifosz /= tx_channels_count;
if (priv->synopsys_id >= DWMAC_CORE_4_00) {
priv->hw->dma->dma_rx_mode(priv->ioaddr, rxmode, chan,
rxfifosz);
- priv->hw->dma->dma_tx_mode(priv->ioaddr, txmode, chan);
+ priv->hw->dma->dma_tx_mode(priv->ioaddr, txmode, chan,
+ txfifosz);
} else {
priv->hw->dma->dma_mode(priv->ioaddr, txmode, rxmode,
rxfifosz);
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 2/2] net: stmmac: Disable flow ctrl for RX AVB queues and really enable TX AVB queues
From: Jose Abreu @ 2017-10-12 15:14 UTC (permalink / raw)
To: netdev
Cc: Jose Abreu, David S. Miller, Joao Pinto, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <cover.1507820933.git.joabreu@synopsys.com>
Flow control must be disabled for AVB enabled queues and TX
AVB queues must be enabled by setting BIT(2) of TXQEN.
Correct this by passing the queue mode to DMA callbacks
and by checking in these functions wether we are in AVB
performing the necessary adjustments.
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
---
drivers/net/ethernet/stmicro/stmmac/common.h | 4 ++--
drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 2 ++
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 16 +++++++++++-----
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++------
4 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index c26c8a7..e1e5ac0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -442,9 +442,9 @@ struct stmmac_dma_ops {
void (*dma_mode)(void __iomem *ioaddr, int txmode, int rxmode,
int rxfifosz);
void (*dma_rx_mode)(void __iomem *ioaddr, int mode, u32 channel,
- int fifosz);
+ int fifosz, u8 qmode);
void (*dma_tx_mode)(void __iomem *ioaddr, int mode, u32 channel,
- int fifosz);
+ int fifosz, u8 qmode);
/* To track extra statistic (if supported) */
void (*dma_diagnostic_fr) (void *data, struct stmmac_extra_stats *x,
void __iomem *ioaddr);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
index d74cedf..aeda3ab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
@@ -225,6 +225,8 @@ enum power_event {
#define MTL_CHAN_RX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x38)
#define MTL_OP_MODE_RSF BIT(5)
+#define MTL_OP_MODE_TXQEN_MASK GENMASK(3, 2)
+#define MTL_OP_MODE_TXQEN_AV BIT(2)
#define MTL_OP_MODE_TXQEN BIT(3)
#define MTL_OP_MODE_TSF BIT(1)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index 898849b..572e96b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -191,7 +191,7 @@ static void dwmac4_rx_watchdog(void __iomem *ioaddr, u32 riwt, u32 number_chan)
}
static void dwmac4_dma_rx_chan_op_mode(void __iomem *ioaddr, int mode,
- u32 channel, int fifosz)
+ u32 channel, int fifosz, u8 qmode)
{
unsigned int rqs = fifosz / 256 - 1;
u32 mtl_rx_op, mtl_rx_int;
@@ -218,8 +218,10 @@ static void dwmac4_dma_rx_chan_op_mode(void __iomem *ioaddr, int mode,
mtl_rx_op &= ~MTL_OP_MODE_RQS_MASK;
mtl_rx_op |= rqs << MTL_OP_MODE_RQS_SHIFT;
- /* enable flow control only if each channel gets 4 KiB or more FIFO */
- if (fifosz >= 4096) {
+ /* Enable flow control only if each channel gets 4 KiB or more FIFO and
+ * only if channel is not an AVB channel.
+ */
+ if ((fifosz >= 4096) && (qmode != MTL_QUEUE_AVB)) {
unsigned int rfd, rfa;
mtl_rx_op |= MTL_OP_MODE_EHFC;
@@ -271,7 +273,7 @@ static void dwmac4_dma_rx_chan_op_mode(void __iomem *ioaddr, int mode,
}
static void dwmac4_dma_tx_chan_op_mode(void __iomem *ioaddr, int mode,
- u32 channel, int fifosz)
+ u32 channel, int fifosz, u8 qmode)
{
u32 mtl_tx_op = readl(ioaddr + MTL_CHAN_TX_OP_MODE(channel));
unsigned int tqs = fifosz / 256 - 1;
@@ -311,7 +313,11 @@ static void dwmac4_dma_tx_chan_op_mode(void __iomem *ioaddr, int mode,
* reflect the available fifo size per queue (total fifo size / number
* of enabled queues).
*/
- mtl_tx_op |= MTL_OP_MODE_TXQEN;
+ mtl_tx_op &= ~MTL_OP_MODE_TXQEN_MASK;
+ if (mtl_tx_op != MTL_QUEUE_AVB)
+ mtl_tx_op |= MTL_OP_MODE_TXQEN;
+ else
+ mtl_tx_op |= MTL_OP_MODE_TXQEN_AV;
mtl_tx_op &= ~MTL_OP_MODE_TQS_MASK;
mtl_tx_op |= tqs << MTL_OP_MODE_TQS_SHIFT;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index edf245b..0e1b0a3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1754,6 +1754,7 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
u32 txmode = 0;
u32 rxmode = 0;
u32 chan = 0;
+ u8 qmode = 0;
if (rxfifosz == 0)
rxfifosz = priv->dma_cap.rx_fifo_size;
@@ -1785,13 +1786,19 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
/* configure all channels */
if (priv->synopsys_id >= DWMAC_CORE_4_00) {
- for (chan = 0; chan < rx_channels_count; chan++)
+ for (chan = 0; chan < rx_channels_count; chan++) {
+ qmode = priv->plat->rx_queues_cfg[chan].mode_to_use;
+
priv->hw->dma->dma_rx_mode(priv->ioaddr, rxmode, chan,
- rxfifosz);
+ rxfifosz, qmode);
+ }
+
+ for (chan = 0; chan < tx_channels_count; chan++) {
+ qmode = priv->plat->tx_queues_cfg[chan].mode_to_use;
- for (chan = 0; chan < tx_channels_count; chan++)
priv->hw->dma->dma_tx_mode(priv->ioaddr, txmode, chan,
- txfifosz);
+ txfifosz, qmode);
+ }
} else {
priv->hw->dma->dma_mode(priv->ioaddr, txmode, rxmode,
rxfifosz);
@@ -1954,6 +1961,8 @@ static void stmmac_tx_err(struct stmmac_priv *priv, u32 chan)
static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
u32 rxmode, u32 chan)
{
+ u8 rxqmode = priv->plat->rx_queues_cfg[chan].mode_to_use;
+ u8 txqmode = priv->plat->tx_queues_cfg[chan].mode_to_use;
u32 rx_channels_count = priv->plat->rx_queues_to_use;
u32 tx_channels_count = priv->plat->tx_queues_to_use;
int rxfifosz = priv->plat->rx_fifo_size;
@@ -1970,9 +1979,9 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
if (priv->synopsys_id >= DWMAC_CORE_4_00) {
priv->hw->dma->dma_rx_mode(priv->ioaddr, rxmode, chan,
- rxfifosz);
+ rxfifosz, rxqmode);
priv->hw->dma->dma_tx_mode(priv->ioaddr, txmode, chan,
- txfifosz);
+ txfifosz, txqmode);
} else {
priv->hw->dma->dma_mode(priv->ioaddr, txmode, rxmode,
rxfifosz);
--
1.9.1
^ permalink raw reply related
* Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement select_queue class_ops
From: Alexander Duyck @ 2017-10-12 15:21 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: Netdev, intel-wired-lan, rodney.cummings, andre.guedes,
Jiri Pirko, ivan.briano, Richard Cochran, henrik,
Jamal Hadi Salim, levipearson, boon.leong.ong, Cong Wang,
Jesus Sanchez-Palencia
In-Reply-To: <20171012005449.26533-3-vinicius.gomes@intel.com>
On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>
> When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
> the netdev_queue pointer that the current child qdisc is associated
> with before creating the new qdisc.
>
> Currently, when using mqprio as root qdisc, the kernel will end up
> getting the queue #0 pointer from the mqprio (root qdisc), which leaves
> any new child qdisc with a possibly wrong netdev_queue pointer.
>
> Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
> issue and avoid an inconsistent state when child qdiscs are replaced.
>
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
> ---
> net/sched/sch_mqprio.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index 6bcdfe6e7b63..8c042ae323e3 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
> }
> }
>
> +static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
> + struct tcmsg *tcm)
> +{
> + return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
> +}
> +
So I was just comparing this against mq_selet_queue, and I was
wondering why we are willing to return NULL here instead of just
returning a pointer to the first Tx queue? I realize there is the fix
in the first patch but it seems like if we are going to go that route
then maybe we should update mq as well so that both of these qdiscs
behave the same way. Either this should work like mq, or mq should
work like this, but we shouldn't have them exposing different
behaviors.
> static const struct Qdisc_class_ops mqprio_class_ops = {
> .graft = mqprio_graft,
> .leaf = mqprio_leaf,
> @@ -403,6 +409,7 @@ static const struct Qdisc_class_ops mqprio_class_ops = {
> .walk = mqprio_walk,
> .dump = mqprio_dump_class,
> .dump_stats = mqprio_dump_class_stats,
> + .select_queue = mqprio_select_queue,
> };
>
> static struct Qdisc_ops mqprio_qdisc_ops __read_mostly = {
> --
> 2.14.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply
* Re: [RFC 0/3] Adding config get/set to devlink
From: Roopa Prabhu @ 2017-10-12 15:31 UTC (permalink / raw)
To: Jiri Pirko
Cc: Steve Lin, netdev@vger.kernel.org, Jiri Pirko,
davem@davemloft.net, Michael Chan, linux-pci, John W. Linville,
Andy Gospodarek
In-Reply-To: <20171012150419.GI14672@nanopsycho>
On Thu, Oct 12, 2017 at 8:04 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>>>> Adds a devlink command for getting & setting device configuration
>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>> attributes. Also introduces an attribute that can be set by a
>>>>> driver to indicate that the config change doesn't take effect
>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>> in this patchset, for which all the configuration changes affect NVM
>>>>> only, and aren't loaded until the next restart.)
>>>>>
>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>
>>>>> Steve Lin (3):
>>>>> devlink: Add config parameter get/set operations
>>>>> bnxt: Move generic devlink code to new file
>>>>> bnxt: Add devlink support for config get/set
>>>>>
>>>>
>>>>Is the goal here to move all ethtool operations to devlink (I saw some
>>>>attrs related to speed etc). ?.
>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>place (and of-course leave the current ethtool api around for backward
>>>>compatibility).
>>>
>>> We need to make sure we are not moving things to devlink which don't
>>> belong there. All options that use "netdev" as a handle should go into
>>> rtnetlink instead.
>>>
>>
>>Any reason you want to keep that restriction ?.
>>FWIS, devlink is a driver api just like ethtool is.
>>and ethtool needs to move to netlink soon...and It would be better to
>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>adding yet another driver api, extending devlink seems like a great
>>fit to me.
>
> Hmm, the original purpose of devlink was to obtain iface for things that
> could not use "netdev" as a handle. I try to stick with it as we already
> have iface for things that could use "netdev" as a handle - rtnetlink.
>
> Not sure we want to go this way and add "netdev"-handle things into
> devlink. Thoughts?
>
Only motivation for me is to keep all driver/hw api in a single place.
and its high time ethtool moved to netlink. I would prefer it be out
of rtnetlink if we have a choice.
Moving some of the driver ops to rtnetlink and leaving the rest in
devlink can be a mess for drivers in the long run.
Maybe we can discuss this more at netdev2.2 ?
^ permalink raw reply
* Re: [RFC 0/3] Adding config get/set to devlink
From: Florian Fainelli @ 2017-10-12 15:43 UTC (permalink / raw)
To: Jiri Pirko, Roopa Prabhu
Cc: Steve Lin, netdev@vger.kernel.org, Jiri Pirko,
davem@davemloft.net, michael.chan, linux-pci, John W. Linville,
gospo
In-Reply-To: <20171012150419.GI14672@nanopsycho>
On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com
>wrote:
>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin
><steven.lin1@broadcom.com> wrote:
>>>>> Adds a devlink command for getting & setting device configuration
>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>> attributes. Also introduces an attribute that can be set by a
>>>>> driver to indicate that the config change doesn't take effect
>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>> in this patchset, for which all the configuration changes affect
>NVM
>>>>> only, and aren't loaded until the next restart.)
>>>>>
>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>
>>>>> Steve Lin (3):
>>>>> devlink: Add config parameter get/set operations
>>>>> bnxt: Move generic devlink code to new file
>>>>> bnxt: Add devlink support for config get/set
>>>>>
>>>>
>>>>Is the goal here to move all ethtool operations to devlink (I saw
>some
>>>>attrs related to speed etc). ?.
>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>place (and of-course leave the current ethtool api around for
>backward
>>>>compatibility).
>>>
>>> We need to make sure we are not moving things to devlink which don't
>>> belong there. All options that use "netdev" as a handle should go
>into
>>> rtnetlink instead.
>>>
>>
>>Any reason you want to keep that restriction ?.
>>FWIS, devlink is a driver api just like ethtool is.
>>and ethtool needs to move to netlink soon...and It would be better to
>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>adding yet another driver api, extending devlink seems like a great
>>fit to me.
>
>Hmm, the original purpose of devlink was to obtain iface for things
>that
>could not use "netdev" as a handle. I try to stick with it as we
>already
>have iface for things that could use "netdev" as a handle - rtnetlink.
>
>Not sure we want to go this way and add "netdev"-handle things into
>devlink. Thoughts?
In the current situation where we have ethtool and devlink operating separately on different objects as entry points to the kernel, I agree with that design.
Once we move ethtool (or however we name its successor) over to netlink there is an opportunity for accessing objects that do and do not have a netdevice representor today (e.g: management ports on switches) with the same interface, and devlink could be used for that.
In terms of compatibility though we should have a pseudo generic layer that can take ethtool ioctl() and transform that into a netlink message and then call back down to drivers with the existing ethtool_ops that are implemented. It is reasonably simple to use coccinelle to update these ethtool_ops with possibly updated signatures to support netlink attributes and whatnot, but forcing drivers to quit doing ethtool_ops entitely and implement new sets of "ethtool over netlink" ops is a non starter IMHO.
--
Florian
^ permalink raw reply
* Re: BUG:af_packet fails to TX TSO frames
From: Anton Ivanov @ 2017-10-12 15:44 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David Miller
In-Reply-To: <f046dbf4-0b46-2d7c-c82c-fe9f50c11f71@cambridgegreys.com>
Found it.
Two bugs canceling each other.
The bind sequence in: psock_txring_vnet.c is wrong.
It does the following addr.sll_protocol = htons(ETH_P_IP);
before calling bind.
If you set addr.sll_protocol to ETH_P_ALL where it should have been in
the first place the test program blows up with -ENOBUFS
I think what is happening is that this value is taken into account when
looking at "what should I use to segment it with" in skb_mac_gso_segment
which is invoked at the end of the verification chain which starts in
packet_direct_xmit in af_packet.c
I have not tried the other test cases like setting it to ETH_P_IP and
giving it IPv6 traffic or the opposite, but my guess is that these will
fail too if they need GSO to be applied.
A.
On 10/12/17 15:12, Anton Ivanov wrote:
>
>
> On 10/12/17 14:39, Willem de Bruijn wrote:
>>> If I produce a real vnet frame out of a live kernel frame using
>>> virtio_net_hdr_from_skb() and try to send it it fails on the check in
>>> af_packet, while succeeding for tap. If I remove the af_packet check
>>> the
>>> frame is accepted by the hardware too.
>>>
>>> If I produce it a synthetic frame + vnet header using the test
>>> program - it
>>> works. Go figure.
>> Besides looking at the raw frame bytes, also compare the setup
>> of virtio_net_header, as well as the tcp checksum field. The stack
>> expects the pseudo header to have already been calculated.
>
> I am feeding it a skb which is coming up in the tx routine of a User
> Mode Linux device which is marked as NETIF_F_HW_CSUM and SG - that
> results in a skb with csum-ed headers, body set up for CSUM_PARTIAL
> and multiple fragments (always at least 1 more frag besides the TCP
> head).
>
> That has everything in order as expected by virtio_net_hdr_from_skb
> and this is what I use to generate the vnet header. It works correctly
> for csum and GRO with af_packet and it works correctly for everything
> using a tap device. It fails only on GSO + af_packet TX.
>
> What I am doing is the same thing virtio_net does - it just takes the
> output of virtio_net_hdr_from_skb and does nothing more. There should
> be no need to do anything more :(
>
> It should just work.
>
> Unless there is a gremlin somewhere in the machinery and that gremlin
> needs some light to be flushed out.
>>
>>> I am going to continue digging into it.
>>>
>>> At the very least I now have a positive test case which uses the same
>>> semantics as my code so I have something to compare to.
>> Glad to hear that the test is helpful. I wrote it because I
>> have run into these exact same issues in the past.
>
> It is. I have changes ready for it so it also supports vector IO, need
> to finish fighting with it.
>
> A.
>
>>
>
--
Anton R. Ivanov
Cambridge Greys Limited, England and Wales company No 10273661
http://www.cambridgegreys.com/
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: Steve Grubb @ 2017-10-12 15:45 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, Linux Containers, Linux API, Linux Audit, Linux FS Devel,
Linux Kernel, Linux Network Development, Simo Sorce,
Carlos O'Donell, Aristeu Rozanski, David Howells,
Eric W. Biederman, Eric Paris, jlayton, Andy Lutomirski, mszeredi,
Paul Moore, Serge E. Hallyn, trondmy, Al Viro
In-Reply-To: <20171012141359.saqdtnodwmbz33b2@madcap2.tricolour.ca>
On Thursday, October 12, 2017 10:14:00 AM EDT Richard Guy Briggs wrote:
> Containers are a userspace concept. The kernel knows nothing of them.
>
> The Linux audit system needs a way to be able to track the container
> provenance of events and actions. Audit needs the kernel's help to do
> this.
>
> Since the concept of a container is entirely a userspace concept, a
> registration from the userspace container orchestration system initiates
> this. This will define a point in time and a set of resources
> associated with a particular container with an audit container ID.
The requirements for common criteria around containers should be very closely
modeled on the requirements for virtualization. It would be the container
manager that is responsible for logging the resource assignment events.
> The registration is a pseudo filesystem (proc, since PID tree already
> exists) write of a u8[16] UUID representing the container ID to a file
> representing a process that will become the first process in a new
> container. This write might place restrictions on mount namespaces
> required to define a container, or at least careful checking of
> namespaces in the kernel to verify permissions of the orchestrator so it
> can't change its own container ID. A bind mount of nsfs may be
> necessary in the container orchestrator's mntNS.
> Note: Use a 128-bit scalar rather than a string to make compares faster
> and simpler.
>
> Require a new CAP_CONTAINER_ADMIN to be able to carry out the
> registration.
Wouldn't CAP_AUDIT_WRITE be sufficient? After all, this is for auditing.
> At that time, record the target container's user-supplied
> container identifier along with the target container's first process
> (which may become the target container's "init" process) process ID
> (referenced from the initial PID namespace), all namespace IDs (in the
> form of a nsfs device number and inode number tuple) in a new auxilliary
> record AUDIT_CONTAINER with a qualifying op=$action field.
This would be in addition to the normal audit fields.
> Issue a new auxilliary record AUDIT_CONTAINER_INFO for each valid
> container ID present on an auditable action or event.
>
> Forked and cloned processes inherit their parent's container ID,
> referenced in the process' task_struct.
>
> Mimic setns(2) and return an error if the process has already initiated
> threading or forked since this registration should happen before the
> process execution is started by the orchestrator and hence should not
> yet have any threads or children. If this is deemed overly restrictive,
> switch all threads and children to the new containerID.
>
> Trust the orchestrator to judiciously use and restrict CAP_CONTAINER_ADMIN.
>
> Log the creation of every namespace, inheriting/adding its spawning
> process' containerID(s), if applicable. Include the spawning and
> spawned namespace IDs (device and inode number tuples).
> [AUDIT_NS_CREATE, AUDIT_NS_DESTROY] [clone(2), unshare(2), setns(2)]
> Note: At this point it appears only network namespaces may need to track
> container IDs apart from processes since incoming packets may cause an
> auditable event before being associated with a process.
>
> Log the destruction of every namespace when it is no longer used by any
> process, include the namespace IDs (device and inode number tuples).
> [AUDIT_NS_DESTROY] [process exit, unshare(2), setns(2)]
In the virtualization requirements, we only log removal of resources when
something is removed by intention. If the VM shuts down, the manager issues a
VIRT_CONTROL stop event and the user space utilities knows this means all
resources have been unassigned.
> Issue a new auxilliary record AUDIT_NS_CHANGE listing (opt: op=$action)
> the parent and child namespace IDs for any changes to a process'
> namespaces. [setns(2)]
> Note: It may be possible to combine AUDIT_NS_* record formats and
> distinguish them with an op=$action field depending on the fields
> required for each message type.
>
> When a container ceases to exist because the last process in that
> container has exited and hence the last namespace has been destroyed and
> its refcount dropping to zero, log the fact.
> (This latter is likely needed for certification accountability.) A
> container object may need a list of processes and/or namespaces.
>
> A namespace cannot directly migrate from one container to another but
> could be assigned to a newly spawned container. A namespace can be
> moved from one container to another indirectly by having that namespace
> used in a second process in another container and then ending all the
> processes in the first container.
I'm thinking that there needs to be a clear delineation between what the
container manager is responsible for and what the kernel needs to do. The
kernel needs the registration system and to associate an identifier with
events inside the container.
But would the container manager be mostly responsible for auditing the events
described here:
https://github.com/linux-audit/audit-documentation/wiki/SPEC-Virtualization-Manager-Guest-Lifecycle-Events
Also, we can already audit exit, unshare, setns, and clone. If the kernel just
sticks the identifier on them, isn't that sufficient?
-Steve
> (v2)
> - switch from u64 to u128 UUID
> - switch from "signal" and "trigger" to "register"
> - restrict registration to single process or force all threads and children
> into same container
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [next-queue PATCH v6 3/5] net/sched: Introduce Credit Based Shaper (CBS) qdisc
From: Jesus Sanchez-Palencia @ 2017-10-12 15:45 UTC (permalink / raw)
To: Henrik Austad, Vinicius Costa Gomes
Cc: netdev, intel-wired-lan, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, boon.leong.ong, richardcochran, levipearson,
rodney.cummings
In-Reply-To: <20171012075826.GA29396@sisyphus.home.austad.us>
Hi Henrik,
On 10/12/2017 12:58 AM, Henrik Austad wrote:
> On Wed, Oct 11, 2017 at 05:54:47PM -0700, Vinicius Costa Gomes wrote:
>> This queueing discipline implements the shaper algorithm defined by
>> the 802.1Q-2014 Section 8.6.8.2 and detailed in Annex L.
>>
>> It's primary usage is to apply some bandwidth reservation to user
>> defined traffic classes, which are mapped to different queues via the
>> mqprio qdisc.
>>
>> Only a simple software implementation is added for now.
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>> ---
>> include/uapi/linux/pkt_sched.h | 18 +++
>> net/sched/Kconfig | 11 ++
>> net/sched/Makefile | 1 +
>> net/sched/sch_cbs.c | 302 +++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 332 insertions(+)
>> create mode 100644 net/sched/sch_cbs.c
>>
>> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
>> index 099bf5528fed..41e349df4bf4 100644
>> --- a/include/uapi/linux/pkt_sched.h
>> +++ b/include/uapi/linux/pkt_sched.h
>> @@ -871,4 +871,22 @@ struct tc_pie_xstats {
>> __u32 maxq; /* maximum queue size */
>> __u32 ecn_mark; /* packets marked with ecn*/
>> };
>> +
>> +/* CBS */
>> +struct tc_cbs_qopt {
>> + __u8 offload;
>> + __s32 hicredit;
>> + __s32 locredit;
>> + __s32 idleslope;
>> + __s32 sendslope;
>> +};
>> +
>> +enum {
>> + TCA_CBS_UNSPEC,
>> + TCA_CBS_PARMS,
>> + __TCA_CBS_MAX,
>> +};
>> +
>> +#define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
>> +
>> #endif
>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>> index e70ed26485a2..c03d86a7775e 100644
>> --- a/net/sched/Kconfig
>> +++ b/net/sched/Kconfig
>> @@ -172,6 +172,17 @@ config NET_SCH_TBF
>> To compile this code as a module, choose M here: the
>> module will be called sch_tbf.
>>
>> +config NET_SCH_CBS
>> + tristate "Credit Based Shaper (CBS)"
>
> Any particular reason why the dependency to MQPRIO was dropped? I'm only
> asking because you added it in v1 of the series and then it disappeared in
> v2 and onwards.
The main reason is that currently there are no qdiscs that depend on any other
specifically. The dependency CBS had on MQPRIO came from how we were calculating
the queue index from the qdisc id inside sch_cbc.c . During the previous review
rounds we agreed on a fix for that and the dependency could be finally removed.
(...)
>> +static int cbs_change(struct Qdisc *sch, struct nlattr *opt)
>> +{
>> + struct cbs_sched_data *q = qdisc_priv(sch);
>> + struct net_device *dev = qdisc_dev(sch);
>> + struct nlattr *tb[TCA_CBS_MAX + 1];
>> + struct ethtool_link_ksettings ecmd;
>> + struct tc_cbs_qopt *qopt;
>> + s64 link_speed;
>> + int err;
>> +
>> + err = nla_parse_nested(tb, TCA_CBS_MAX, opt, cbs_policy, NULL);
>> + if (err < 0)
>> + return err;
>> +
>> + if (!tb[TCA_CBS_PARMS])
>> + return -EINVAL;
>> +
>> + qopt = nla_data(tb[TCA_CBS_PARMS]);
>> +
>> + if (qopt->offload)
>> + return -EOPNOTSUPP;
>> +
>> + if (!__ethtool_get_link_ksettings(dev, &ecmd))
>> + link_speed = ecmd.base.speed;
>> + else
>> + link_speed = SPEED_1000;
>> +
>> + q->port_rate = link_speed * 1000 * BYTES_PER_KBIT;
>> +
>> + q->enqueue = cbs_enqueue_soft;
>> + q->dequeue = cbs_dequeue_soft;
>
> How does one use the cbs_(en|de)queue instead of _soft()?
Judging from your comment on the other patch I believe you've got that
clarified. Please let us know if otherwise.
Thanks,
Jesus
^ permalink raw reply
* Re: BUG:af_packet fails to TX TSO frames
From: Anton Ivanov @ 2017-10-12 15:57 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David Miller, user-mode-linux-devel
In-Reply-To: <e301fbfd-a283-caa1-5915-8be15677ed74@cambridgegreys.com>
Also confirmed using my UML patchset. This is inside the UML guest.
root@blinky:~# iperf -c 192.168.98.1
------------------------------------------------------------
Client connecting to 192.168.98.1, TCP port 5001
TCP window size: 414 KByte (default)
------------------------------------------------------------
[ 3] local 192.168.98.146 port 36744 connected with 192.168.98.1 port 5001
[ ID] Interval Transfer Bandwidth
[ 3] 0.0-10.0 sec 4.52 GBytes 3.89 Gbits/sec
This is GSO on a raw socket Virtual NIC to host. Normal speed without
GSO on same machine is ~ 500.
So for the time being I can turn off TSO for anything but v4 in the User
Mode Linux patchset (declare the guest NIC as TSOV4 capable only) and
enable it once the host kernel is fixed. Alternatively, there is the
rather ugly approach of using multiple FDs - one for v4, one for v6, one
for...
A.
On 10/12/17 16:44, Anton Ivanov wrote:
> Found it.
>
> Two bugs canceling each other.
> The bind sequence in: psock_txring_vnet.c is wrong.
>
> It does the following addr.sll_protocol = htons(ETH_P_IP);
> before calling bind.
>
> If you set addr.sll_protocol to ETH_P_ALL where it should have been in
> the first place the test program blows up with -ENOBUFS
>
> I think what is happening is that this value is taken into account
> when looking at "what should I use to segment it with" in
> skb_mac_gso_segment which is invoked at the end of the verification
> chain which starts in packet_direct_xmit in af_packet.c
>
> I have not tried the other test cases like setting it to ETH_P_IP and
> giving it IPv6 traffic or the opposite, but my guess is that these
> will fail too if they need GSO to be applied.
>
> A.
>
> On 10/12/17 15:12, Anton Ivanov wrote:
>>
>>
>> On 10/12/17 14:39, Willem de Bruijn wrote:
>>>> If I produce a real vnet frame out of a live kernel frame using
>>>> virtio_net_hdr_from_skb() and try to send it it fails on the check in
>>>> af_packet, while succeeding for tap. If I remove the af_packet
>>>> check the
>>>> frame is accepted by the hardware too.
>>>>
>>>> If I produce it a synthetic frame + vnet header using the test
>>>> program - it
>>>> works. Go figure.
>>> Besides looking at the raw frame bytes, also compare the setup
>>> of virtio_net_header, as well as the tcp checksum field. The stack
>>> expects the pseudo header to have already been calculated.
>>
>> I am feeding it a skb which is coming up in the tx routine of a User
>> Mode Linux device which is marked as NETIF_F_HW_CSUM and SG - that
>> results in a skb with csum-ed headers, body set up for CSUM_PARTIAL
>> and multiple fragments (always at least 1 more frag besides the TCP
>> head).
>>
>> That has everything in order as expected by virtio_net_hdr_from_skb
>> and this is what I use to generate the vnet header. It works
>> correctly for csum and GRO with af_packet and it works correctly for
>> everything using a tap device. It fails only on GSO + af_packet TX.
>>
>> What I am doing is the same thing virtio_net does - it just takes the
>> output of virtio_net_hdr_from_skb and does nothing more. There should
>> be no need to do anything more :(
>>
>> It should just work.
>>
>> Unless there is a gremlin somewhere in the machinery and that gremlin
>> needs some light to be flushed out.
>>>
>>>> I am going to continue digging into it.
>>>>
>>>> At the very least I now have a positive test case which uses the same
>>>> semantics as my code so I have something to compare to.
>>> Glad to hear that the test is helpful. I wrote it because I
>>> have run into these exact same issues in the past.
>>
>> It is. I have changes ready for it so it also supports vector IO,
>> need to finish fighting with it.
>>
>> A.
>>
>>>
>>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
^ permalink raw reply
* Re: [RFC 0/3] Adding config get/set to devlink
From: Roopa Prabhu @ 2017-10-12 16:05 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jiri Pirko, Steve Lin, netdev@vger.kernel.org, Jiri Pirko,
davem@davemloft.net, Michael Chan, linux-pci, John W. Linville,
Andy Gospodarek
In-Reply-To: <24E5DE7C-A401-48BF-BF80-673ACC38FBBE@gmail.com>
On Thu, Oct 12, 2017 at 8:43 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On October 12, 2017 8:04:19 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>>Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>>>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com
>>wrote:
>>>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin
>><steven.lin1@broadcom.com> wrote:
>>>>>> Adds a devlink command for getting & setting device configuration
>>>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>>>> attributes. Also introduces an attribute that can be set by a
>>>>>> driver to indicate that the config change doesn't take effect
>>>>>> until the next restart (as in the case of the bnxt driver changes
>>>>>> in this patchset, for which all the configuration changes affect
>>NVM
>>>>>> only, and aren't loaded until the next restart.)
>>>>>>
>>>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>>>
>>>>>> Steve Lin (3):
>>>>>> devlink: Add config parameter get/set operations
>>>>>> bnxt: Move generic devlink code to new file
>>>>>> bnxt: Add devlink support for config get/set
>>>>>>
>>>>>
>>>>>Is the goal here to move all ethtool operations to devlink (I saw
>>some
>>>>>attrs related to speed etc). ?.
>>>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>>>place (and of-course leave the current ethtool api around for
>>backward
>>>>>compatibility).
>>>>
>>>> We need to make sure we are not moving things to devlink which don't
>>>> belong there. All options that use "netdev" as a handle should go
>>into
>>>> rtnetlink instead.
>>>>
>>>
>>>Any reason you want to keep that restriction ?.
>>>FWIS, devlink is a driver api just like ethtool is.
>>>and ethtool needs to move to netlink soon...and It would be better to
>>>not put the rtnl_lock burden on ethtool driver operations. Instead of
>>>adding yet another driver api, extending devlink seems like a great
>>>fit to me.
>>
>>Hmm, the original purpose of devlink was to obtain iface for things
>>that
>>could not use "netdev" as a handle. I try to stick with it as we
>>already
>>have iface for things that could use "netdev" as a handle - rtnetlink.
>>
>>Not sure we want to go this way and add "netdev"-handle things into
>>devlink. Thoughts?
>
> In the current situation where we have ethtool and devlink operating separately on different objects as entry points to the kernel, I agree with that design.
>
> Once we move ethtool (or however we name its successor) over to netlink there is an opportunity for accessing objects that do and do not have a netdevice representor today (e.g: management ports on switches) with the same interface, and devlink could be used for that.
>
> In terms of compatibility though we should have a pseudo generic layer that can take ethtool ioctl() and transform that into a netlink message and then call back down to drivers with the existing ethtool_ops that are implemented. It is reasonably simple to use coccinelle to update these ethtool_ops with possibly updated signatures to support netlink attributes and whatnot,
ack, that sounds like a good approach.
> but forcing drivers to quit doing ethtool_ops entitely and implement new sets of "ethtool over netlink" ops is a non starter IMHO.
correct, nobody disagrees with that point.
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement select_queue class_ops
From: Jesus Sanchez-Palencia @ 2017-10-12 15:59 UTC (permalink / raw)
To: Alexander Duyck, Vinicius Costa Gomes
Cc: Netdev, intel-wired-lan, rodney.cummings, andre.guedes,
Jiri Pirko, ivan.briano, Richard Cochran, henrik,
Jamal Hadi Salim, levipearson, boon.leong.ong, Cong Wang
In-Reply-To: <CAKgT0Ufo1tG4GpSkGaWuUxqhzEUOT7wcBLuaZx_mZmEL+9SnXw@mail.gmail.com>
Hi Alex,
On 10/12/2017 08:21 AM, Alexander Duyck wrote:
> On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>>
>> When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
>> the netdev_queue pointer that the current child qdisc is associated
>> with before creating the new qdisc.
>>
>> Currently, when using mqprio as root qdisc, the kernel will end up
>> getting the queue #0 pointer from the mqprio (root qdisc), which leaves
>> any new child qdisc with a possibly wrong netdev_queue pointer.
>>
>> Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
>> issue and avoid an inconsistent state when child qdiscs are replaced.
>>
>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>> ---
>> net/sched/sch_mqprio.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
>> index 6bcdfe6e7b63..8c042ae323e3 100644
>> --- a/net/sched/sch_mqprio.c
>> +++ b/net/sched/sch_mqprio.c
>> @@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>> }
>> }
>>
>> +static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
>> + struct tcmsg *tcm)
>> +{
>> + return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
>> +}
>> +
>
> So I was just comparing this against mq_selet_queue, and I was
> wondering why we are willing to return NULL here instead of just
> returning a pointer to the first Tx queue? I realize there is the fix
> in the first patch but it seems like if we are going to go that route
> then maybe we should update mq as well so that both of these qdiscs
> behave the same way. Either this should work like mq, or mq should
> work like this, but we shouldn't have them exposing different
> behaviors.
This was brought up by Cong Wang during the review of our v2. Based on my
understanding, the point I've made is that for mqprio the inner qdiscs are
always 'related' to one of the Tx netdev_queues per design. Returning any other
queue as a fallback seemed like going against that to me.
I'm still inclined to say that we should keep this function as the patch is
proposing, thus either returning the correct netdev_queue for a given handle, or
NULL as a way to flag that something was 'wrong' with it. Returning queue #0 is
misleading in that sense, imho.
As for aligning mq_select_queue() with this approach, if my reasoning behind
mqprio is correct and also applies to mq, I would be happy to send that fix as
part our v7.
What do you think?
Thanks,
Jesus
^ permalink raw reply
* Re: [PATCHv4 iproute2 2/2] lib/libnetlink: update rtnl_talk to support malloc buff at run time
From: Stephen Hemminger @ 2017-10-12 16:07 UTC (permalink / raw)
To: Phil Sutter; +Cc: Michal Kubecek, Hangbin Liu, netdev, Hangbin Liu
In-Reply-To: <20171011111007.GA11332@orbyte.nwl.cc>
On Wed, 11 Oct 2017 13:10:07 +0200
Phil Sutter <phil@nwl.cc> wrote:
> On Tue, Oct 10, 2017 at 09:47:43AM -0700, Stephen Hemminger wrote:
> > On Tue, 10 Oct 2017 08:41:17 +0200
> > Michal Kubecek <mkubecek@suse.cz> wrote:
> >
> > > On Mon, Oct 09, 2017 at 10:25:25PM +0200, Phil Sutter wrote:
> > > > Hi Stephen,
> > > >
> > > > On Mon, Oct 02, 2017 at 10:37:08AM -0700, Stephen Hemminger wrote:
> > > > > On Thu, 28 Sep 2017 21:33:46 +0800
> > > > > Hangbin Liu <haliu@redhat.com> wrote:
> > > > >
> > > > > > From: Hangbin Liu <liuhangbin@gmail.com>
> > > > > >
> > > > > > This is an update for 460c03f3f3cc ("iplink: double the buffer size also in
> > > > > > iplink_get()"). After update, we will not need to double the buffer size
> > > > > > every time when VFs number increased.
> > > > > >
> > > > > > With call like rtnl_talk(&rth, &req.n, NULL, 0), we can simply remove the
> > > > > > length parameter.
> > > > > >
> > > > > > With call like rtnl_talk(&rth, nlh, nlh, sizeof(req), I add a new variable
> > > > > > answer to avoid overwrite data in nlh, because it may has more info after
> > > > > > nlh. also this will avoid nlh buffer not enough issue.
> > > > > >
> > > > > > We need to free answer after using.
> > > > > >
> > > > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > > ---
> > > > >
> > > > > Most of the uses of rtnl_talk() don't need to this peek and dynamic sizing.
> > > > > Can only those places that need that be targeted?
> > > >
> > > > We could probably do that, by having a buffer on stack in __rtnl_talk()
> > > > which will be used instead of the allocated one if 'answer' is NULL. Or
> > > > maybe even introduce a dedicated API call for the dynamically allocated
> > > > receive buffer. But I really doubt that's feasible: AFAICT, that stack
> > > > buffer still needs to be reasonably sized since the reply might be
> > > > larger than the request (reusing the request buffer would be the most
> > > > simple way to tackle this), also there is support for extack which may
> > > > bloat the response to arbitrary size. Hangbin has shown in his benchmark
> > > > that the overhead of the second syscall is negligible, so why care about
> > > > that and increase code complexity even further?
> > > >
> > > > Not saying it's not possible, but I just doubt it's worth the effort.
> > >
> > > Agreed. Current code is based on the assumption that we can estimate the
> > > maximum reply length in advance and the reason for this series is that
> > > this assumption turned out to be wrong. I'm afraid that if we replace
> > > it by an assumption that we can estimate the maximum reply length for
> > > most requests with only few exceptions, it's only matter of time for us
> > > to be proven wrong again.
> > >
> > > Michal Kubecek
> > >
> >
> > For query responses, yes the response may be large. But for the common cases of
> > add address or add route, the response should just be ack or error.
>
> And with extack, error is comprised of the original request plus an
> arbitrarily sized error message, so we can't just reuse the request
> buffer and are back to "guessing" the right length again.
>
> To get an idea of what we're talking about, I wrote a simple benchmark
> which adds 256 * 254 (= 65024) addresses to an interface, then removes
> them again one by one and measured the time that takes for binaries with
> and without Hangbin's patches:
>
> OP Vanilla Hangbin Delta
> --------------------------------------------------------
> add real 2m16.244s real 2m27.964s +11.72s (108.6%)
> user 0m15.241s user 0m17.295s +2.054s (113.5%)
> sys 1m40.229s sys 1m48.239s +8.01s (108.0%)
>
> remove real 1m44.950s real 1m47.044s +2.094s (102.0%)
> user 0m13.899s user 0m14.723s +0.824s (105.9%)
> sys 1m30.798s sys 1m31.938s +1.140s (101.3%)
>
> So the overhead of the second syscall and dynamic memory allocation is
> less than 10% overall. Given the short time a single call to 'ip'
> typically takes, I don't think the difference is noticeable even in
> highly performance critical applications.
>
> Cheers, Phil
For a better benchmark, I generated 4 Million routes
then did:
# ip ---batch routes.txt
OP Vanilla Hangbin Delta
-----------------------------------------------------
add real 1:25.840 1:33.677 +9.13%
user 10.690 6.078 -56.85%
sys 1:00.920 1:13.109 +20.00%
remove real 2:29.881 2:25.872 -2.67%
user 12.862 7.942 -38.25%
sys 44.127 44.633 +1.15%
So the answer is addition is slower but deletion appears faster?
If I rerun the Vanilla test, get about the same times.
The slowdown won't impact me, but what about large scale users
like Cumulus.
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement select_queue class_ops
From: Alexander Duyck @ 2017-10-12 16:09 UTC (permalink / raw)
To: Jesus Sanchez-Palencia
Cc: Vinicius Costa Gomes, Netdev, intel-wired-lan, rodney.cummings,
andre.guedes, Jiri Pirko, ivan.briano, Richard Cochran, henrik,
Jamal Hadi Salim, levipearson, boon.leong.ong, Cong Wang
In-Reply-To: <633dfce2-71f1-f704-7ae2-b1ef2ba9b448@intel.com>
On Thu, Oct 12, 2017 at 8:59 AM, Jesus Sanchez-Palencia
<jesus.sanchez-palencia@intel.com> wrote:
> Hi Alex,
>
>
> On 10/12/2017 08:21 AM, Alexander Duyck wrote:
>> On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
>> <vinicius.gomes@intel.com> wrote:
>>> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>>>
>>> When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
>>> the netdev_queue pointer that the current child qdisc is associated
>>> with before creating the new qdisc.
>>>
>>> Currently, when using mqprio as root qdisc, the kernel will end up
>>> getting the queue #0 pointer from the mqprio (root qdisc), which leaves
>>> any new child qdisc with a possibly wrong netdev_queue pointer.
>>>
>>> Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
>>> issue and avoid an inconsistent state when child qdiscs are replaced.
>>>
>>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>>> ---
>>> net/sched/sch_mqprio.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
>>> index 6bcdfe6e7b63..8c042ae323e3 100644
>>> --- a/net/sched/sch_mqprio.c
>>> +++ b/net/sched/sch_mqprio.c
>>> @@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>>> }
>>> }
>>>
>>> +static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
>>> + struct tcmsg *tcm)
>>> +{
>>> + return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
>>> +}
>>> +
>>
>> So I was just comparing this against mq_selet_queue, and I was
>> wondering why we are willing to return NULL here instead of just
>> returning a pointer to the first Tx queue? I realize there is the fix
>> in the first patch but it seems like if we are going to go that route
>> then maybe we should update mq as well so that both of these qdiscs
>> behave the same way. Either this should work like mq, or mq should
>> work like this, but we shouldn't have them exposing different
>> behaviors.
>
>
> This was brought up by Cong Wang during the review of our v2. Based on my
> understanding, the point I've made is that for mqprio the inner qdiscs are
> always 'related' to one of the Tx netdev_queues per design. Returning any other
> queue as a fallback seemed like going against that to me.
>
> I'm still inclined to say that we should keep this function as the patch is
> proposing, thus either returning the correct netdev_queue for a given handle, or
> NULL as a way to flag that something was 'wrong' with it. Returning queue #0 is
> misleading in that sense, imho.
>
> As for aligning mq_select_queue() with this approach, if my reasoning behind
> mqprio is correct and also applies to mq, I would be happy to send that fix as
> part our v7.
>
> What do you think?
>
>
> Thanks,
> Jesus
I think it would be better to bring mq_select_queue in line with your
fix. You could probably just add it to your first patch. That way if
the user specifies a bad qdisc classid they don't get to just
overwrite the qdisc on Tx queue 0.
- Alex
^ permalink raw reply
* RE: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: David Laight @ 2017-10-12 16:20 UTC (permalink / raw)
To: 'Timur Tabi', David S. Miller, netdev@vger.kernel.org
In-Reply-To: <e523734d-d845-8eae-4a5b-e679b8e46654@codeaurora.org>
From: Timur Tabi
> Sent: 12 October 2017 15:13
> On 10/12/17 4:30 AM, David Laight wrote:
> > Isn't the memory allocated by a single kzalloc() call?
>
> dma_alloc_coherenent, actually.
>
> > IIRC that guarantees it doesn't cross a power or 2 boundary less than
> > the size.
>
> I'm pretty sure that kzalloc does not make that guarantee, and I don't
> think dma_alloc_coherent does either.
dma_alloc_coherent() definitely does.
And I've a driver that relies on it (for 16k blocks).
David
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement select_queue class_ops
From: Jesus Sanchez-Palencia @ 2017-10-12 16:16 UTC (permalink / raw)
To: Alexander Duyck
Cc: Vinicius Costa Gomes, Netdev, intel-wired-lan, rodney.cummings,
andre.guedes, Jiri Pirko, ivan.briano, Richard Cochran, henrik,
Jamal Hadi Salim, levipearson, boon.leong.ong, Cong Wang
In-Reply-To: <CAKgT0UcpSq2-LaqDKhTWKoZdemDE2SOCWggxGBjCH98qYPBGFw@mail.gmail.com>
On 10/12/2017 09:09 AM, Alexander Duyck wrote:
> On Thu, Oct 12, 2017 at 8:59 AM, Jesus Sanchez-Palencia
> <jesus.sanchez-palencia@intel.com> wrote:
>> Hi Alex,
>>
>>
>> On 10/12/2017 08:21 AM, Alexander Duyck wrote:
>>> On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
>>> <vinicius.gomes@intel.com> wrote:
>>>> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>>>>
>>>> When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
>>>> the netdev_queue pointer that the current child qdisc is associated
>>>> with before creating the new qdisc.
>>>>
>>>> Currently, when using mqprio as root qdisc, the kernel will end up
>>>> getting the queue #0 pointer from the mqprio (root qdisc), which leaves
>>>> any new child qdisc with a possibly wrong netdev_queue pointer.
>>>>
>>>> Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
>>>> issue and avoid an inconsistent state when child qdiscs are replaced.
>>>>
>>>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>>>> ---
>>>> net/sched/sch_mqprio.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
>>>> index 6bcdfe6e7b63..8c042ae323e3 100644
>>>> --- a/net/sched/sch_mqprio.c
>>>> +++ b/net/sched/sch_mqprio.c
>>>> @@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>>>> }
>>>> }
>>>>
>>>> +static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
>>>> + struct tcmsg *tcm)
>>>> +{
>>>> + return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
>>>> +}
>>>> +
>>>
>>> So I was just comparing this against mq_selet_queue, and I was
>>> wondering why we are willing to return NULL here instead of just
>>> returning a pointer to the first Tx queue? I realize there is the fix
>>> in the first patch but it seems like if we are going to go that route
>>> then maybe we should update mq as well so that both of these qdiscs
>>> behave the same way. Either this should work like mq, or mq should
>>> work like this, but we shouldn't have them exposing different
>>> behaviors.
>>
>>
>> This was brought up by Cong Wang during the review of our v2. Based on my
>> understanding, the point I've made is that for mqprio the inner qdiscs are
>> always 'related' to one of the Tx netdev_queues per design. Returning any other
>> queue as a fallback seemed like going against that to me.
>>
>> I'm still inclined to say that we should keep this function as the patch is
>> proposing, thus either returning the correct netdev_queue for a given handle, or
>> NULL as a way to flag that something was 'wrong' with it. Returning queue #0 is
>> misleading in that sense, imho.
>>
>> As for aligning mq_select_queue() with this approach, if my reasoning behind
>> mqprio is correct and also applies to mq, I would be happy to send that fix as
>> part our v7.
>>
>> What do you think?
>>
>>
>> Thanks,
>> Jesus
>
> I think it would be better to bring mq_select_queue in line with your
> fix. You could probably just add it to your first patch. That way if
> the user specifies a bad qdisc classid they don't get to just
> overwrite the qdisc on Tx queue 0.
Ok, I will send the fix then, but I'm just not sure if I'll send it together
with the patch fixing qdisc_alloc(). Looks like a change that should be together
with this one of after it, instead.
Anyhow, it will be fixed in our v7. Thanks!
^ permalink raw reply
* Re: BUG:af_packet fails to TX TSO frames
From: Willem de Bruijn @ 2017-10-12 16:30 UTC (permalink / raw)
To: Anton Ivanov; +Cc: Network Development, David Miller
In-Reply-To: <e301fbfd-a283-caa1-5915-8be15677ed74@cambridgegreys.com>
On Thu, Oct 12, 2017 at 11:44 AM, Anton Ivanov
<anton.ivanov@cambridgegreys.com> wrote:
> Found it.
>
> Two bugs canceling each other.
> The bind sequence in: psock_txring_vnet.c is wrong.
>
> It does the following addr.sll_protocol = htons(ETH_P_IP);
> before calling bind.
>
> If you set addr.sll_protocol to ETH_P_ALL where it should have been in the
> first place the test program blows up with -ENOBUFS
There is no such requirement that the socket should bind to ETH_P_ALL.
> I think what is happening is that this value is taken into account when
> looking at "what should I use to segment it with" in skb_mac_gso_segment
> which is invoked at the end of the verification chain which starts in
> packet_direct_xmit in af_packet.c
packet_snd sets skb->protocol based on the protocol that the packet
socket is bound to. Binding to ETH_P_IP is the right choice here.
^ permalink raw reply
* Re: RFC(v2): Audit Kernel Container IDs
From: Casey Schaufler @ 2017-10-12 16:33 UTC (permalink / raw)
To: Richard Guy Briggs, cgroups-u79uwXL29TY76Z2rM5mHXA,
Linux Containers, Linux API, Linux Audit, Linux FS Devel,
Linux Kernel, Linux Network Development
Cc: mszeredi-H+wXaHxf7aLQT0dZR+AlfA, Andy Lutomirski,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, Carlos O'Donell, Al Viro,
David Howells, Simo Sorce, trondmy-7I+n7zu2hftEKMMhf/gKZA,
Eric Paris, Serge E. Hallyn, Eric W. Biederman
In-Reply-To: <20171012141359.saqdtnodwmbz33b2-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On 10/12/2017 7:14 AM, Richard Guy Briggs wrote:
> Containers are a userspace concept. The kernel knows nothing of them.
>
> The Linux audit system needs a way to be able to track the container
> provenance of events and actions. Audit needs the kernel's help to do
> this.
>
> Since the concept of a container is entirely a userspace concept, a
> registration from the userspace container orchestration system initiates
> this. This will define a point in time and a set of resources
> associated with a particular container with an audit container ID.
>
> The registration is a pseudo filesystem (proc, since PID tree already
> exists) write of a u8[16] UUID representing the container ID to a file
> representing a process that will become the first process in a new
> container. This write might place restrictions on mount namespaces
> required to define a container, or at least careful checking of
> namespaces in the kernel to verify permissions of the orchestrator so it
> can't change its own container ID. A bind mount of nsfs may be
> necessary in the container orchestrator's mntNS.
> Note: Use a 128-bit scalar rather than a string to make compares faster
> and simpler.
>
> Require a new CAP_CONTAINER_ADMIN to be able to carry out the
> registration.
Hang on. If containers are a user space concept, how can
you want CAP_CONTAINER_ANYTHING? If there's not such thing as
a container, how can you be asking for a capability to manage
them?
> At that time, record the target container's user-supplied
> container identifier along with the target container's first process
> (which may become the target container's "init" process) process ID
> (referenced from the initial PID namespace), all namespace IDs (in the
> form of a nsfs device number and inode number tuple) in a new auxilliary
> record AUDIT_CONTAINER with a qualifying op=$action field.
>
> Issue a new auxilliary record AUDIT_CONTAINER_INFO for each valid
> container ID present on an auditable action or event.
>
> Forked and cloned processes inherit their parent's container ID,
> referenced in the process' task_struct.
>
> Mimic setns(2) and return an error if the process has already initiated
> threading or forked since this registration should happen before the
> process execution is started by the orchestrator and hence should not
> yet have any threads or children. If this is deemed overly restrictive,
> switch all threads and children to the new containerID.
>
> Trust the orchestrator to judiciously use and restrict CAP_CONTAINER_ADMIN.
>
> Log the creation of every namespace, inheriting/adding its spawning
> process' containerID(s), if applicable. Include the spawning and
> spawned namespace IDs (device and inode number tuples).
> [AUDIT_NS_CREATE, AUDIT_NS_DESTROY] [clone(2), unshare(2), setns(2)]
> Note: At this point it appears only network namespaces may need to track
> container IDs apart from processes since incoming packets may cause an
> auditable event before being associated with a process.
>
> Log the destruction of every namespace when it is no longer used by any
> process, include the namespace IDs (device and inode number tuples).
> [AUDIT_NS_DESTROY] [process exit, unshare(2), setns(2)]
>
> Issue a new auxilliary record AUDIT_NS_CHANGE listing (opt: op=$action)
> the parent and child namespace IDs for any changes to a process'
> namespaces. [setns(2)]
> Note: It may be possible to combine AUDIT_NS_* record formats and
> distinguish them with an op=$action field depending on the fields
> required for each message type.
>
> When a container ceases to exist because the last process in that
> container has exited and hence the last namespace has been destroyed and
> its refcount dropping to zero, log the fact.
> (This latter is likely needed for certification accountability.) A
> container object may need a list of processes and/or namespaces.
>
> A namespace cannot directly migrate from one container to another but
> could be assigned to a newly spawned container. A namespace can be
> moved from one container to another indirectly by having that namespace
> used in a second process in another container and then ending all the
> processes in the first container.
>
> (v2)
> - switch from u64 to u128 UUID
> - switch from "signal" and "trigger" to "register"
> - restrict registration to single process or force all threads and children into same container
>
> - RGB
>
> --
> Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> https://www.redhat.com/mailman/listinfo/linux-audit
>
^ permalink raw reply
* Re: [PATCH net-next 1/1] veth: tweak creation of veth device
From: Roman Mashak @ 2017-10-12 16:49 UTC (permalink / raw)
To: David Miller; +Cc: jhs, netdev
In-Reply-To: <20171011.151706.1844884518098480593.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
>> When creating veth pair, at first rtnl_new_link() creates veth_dev, i.e.
>> one end of the veth pipe, but not registers it; then veth_newlink() gets
>> invoked, where peer dev is created _and_ registered, followed by veth_dev
>> registration, which may fail if peer information, that is VETH_INFO_PEER
>> attribute, has not been provided and the kernel will allocate unique veth
>> name.
>>
>> So, we should ask the kernel to allocate unique name for veth_dev only
>> when peer info is not available.
>>
>> Example:
>>
>> % ip link dev veth0 type veth
>> RTNETLINK answers: File exists
>>
>> After fix:
>> % ip link dev veth0 type veth
>> % ip link show dev veth0
>> 5: veth0@veth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>> link/ether f6:ef:8b:96:f4:ec brd ff:ff:ff:ff:ff:ff
>> %
>>
>> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
>
> I'm not so sure about this.
>
> If we specify an explicit tb[IFLA_NAME], we shouldn't completely ignore that
> request from the user just because they didn't give any peer information.
>
> I see what happens in this case, the peer gets 'veth0' and then since
> the user asked for 'veth0' for the non-peer it conflicts.
So, the only way is to require user space to _always_ pass in
VETH_INFO_PEER, which may break existing code (fixing iproute2 is easiest).
Otherwise ignore netlink messages lacking of VETH_INFO_PEER and return
error.
IMO, neither of these solutions seem reasonable.
Also, there are valid use cases where a user does not care about veth
name sitting in container, but assigns a name following certain
pattern to a host-side veth.
> Well, too bad. The user must work to orchestrate things such that
> this doesn't happen. That means either providing the IFLA_NAME for
> both the peer and the non-peer, or specifying neither.
>
> I'm not applying this, sorry.
^ permalink raw reply
* Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: Timur Tabi @ 2017-10-12 16:52 UTC (permalink / raw)
To: David Laight, David S. Miller, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD0092ADF@AcuExch.aculab.com>
On 10/12/2017 11:20 AM, David Laight wrote:
>> I'm pretty sure that kzalloc does not make that guarantee, and I don't
>> think dma_alloc_coherent does either.
> dma_alloc_coherent() definitely does.
> And I've a driver that relies on it (for 16k blocks).
What about when an IOMMU is used? The DMA address that gets returned is
not necessarily the physical address of the memory buffer.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: David Miller @ 2017-10-12 16:58 UTC (permalink / raw)
To: timur; +Cc: David.Laight, netdev
In-Reply-To: <e523734d-d845-8eae-4a5b-e679b8e46654@codeaurora.org>
From: Timur Tabi <timur@codeaurora.org>
Date: Thu, 12 Oct 2017 09:13:25 -0500
> On 10/12/17 4:30 AM, David Laight wrote:
>> Isn't the memory allocated by a single kzalloc() call?
>
> dma_alloc_coherenent, actually.
>
>> IIRC that guarantees it doesn't cross a power or 2 boundary less than
>> the size.
>
> I'm pretty sure that kzalloc does not make that guarantee, and I don't
> think dma_alloc_coherent does either.
Both make that guarantee, even when an IOMMU is used.
^ permalink raw reply
* Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: Timur Tabi @ 2017-10-12 17:15 UTC (permalink / raw)
To: David Miller; +Cc: David.Laight, netdev
In-Reply-To: <20171012.095837.2057549694773237248.davem@davemloft.net>
On 10/12/2017 11:58 AM, David Miller wrote:
>> I'm pretty sure that kzalloc does not make that guarantee, and I don't
>> think dma_alloc_coherent does either.
> Both make that guarantee, even when an IOMMU is used.
>
Ok, Dave, then can you drop this patch?
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* [patch net-next 00/34] net: sched: allow qdiscs to share filter block instances
From: Jiri Pirko @ 2017-10-12 17:17 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
matanb, leonro, idosch, jakub.kicinski, ast, daniel, simon.horman,
pieter.jansenvanvuuren, john.hurley, edumazet, dsahern,
alexander.h.duyck, john.fastabend, willemb
From: Jiri Pirko <jiri@mellanox.com>
First of all, I would like to apologize for big patchset. However after
couple of hours trying to figure out how to cut it, I found out it is
actually not possible. I would have to add some interface in one patchset
and only use it in second, which is forbidden. Also, I would like
to provide the reviewer the full picture. Most of the patches are small
and contained anyway, so it should be easy to review them. But to the
motivation:
Currently the filters added to qdiscs are independent. So for example if you
have 2 netdevices and you create ingress qdisc on both and you want to add
identical filter rules both, you need to add them twice. This patchset
makes this easier and mainly saves resources allowing to share all filters
within a qdisc - I call it a "filter block". Also this helps to save
resources when we do offload to hw for example to expensive TCAM.
So back to the example. First, we create 2 qdiscs. Both will share
block number 22. "22" is just an identification. If we don't pass any
block number, a new one will be generated by kernel:
$ tc qdisc add dev ens7 ingress block 22
^^^^^^^^
$ tc qdisc add dev ens8 ingress block 22
^^^^^^^^
Now if we list the qdiscs, we will see the block index in the output:
$ tc qdisc
qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
qdisc ingress ffff: dev ens8 parent ffff:fff1 block 22
Now we can add filter to any of qdiscs sharing the same block:
$ tc filter add dev ens7 parent ffff: protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
We will see the same output if we list filters for ens7 and ens8, including stats:
$ tc -s filter show dev ens7 ingress
filter protocol ip pref 25 flower chain 0
filter protocol ip pref 25 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 192.168.0.0/16
not_in_hw
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1 installed 39 sec used 2 sec
Action statistics:
Sent 3108 bytes 37 pkt (dropped 37, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
$ tc -s filter show dev ens8 ingress
filter protocol ip pref 25 flower chain 0
filter protocol ip pref 25 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 192.168.0.0/16
not_in_hw
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1 installed 40 sec used 3 sec
Action statistics:
Sent 3108 bytes 37 pkt (dropped 37, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
Patches overview:
Patches 1-3 introduce infrastructure for block sharing and the interface
funtions to the qdisc, tcf_block_get_ext and tcf_block_put_ext
Patches 4-11 are removing usages of tp->q pointer, which needs to be
eventually removed in order to set the tfc_proto independent
on a qdisc instance
Patches 12-19 introduces block callbacks, internal infra and driver-facing
interface, they add callback calling to individual classifiers
Patches 20-28 convert individual drivers from ndo_setup_tc to block
callbacks for classifiers offloading
Patches 29-31 remove unused things due to the previous conversion
Patch 32 introduces block mechanism to handle netif_keep_dst calls
Patch 33 removes tp->q and tp->classid - makes tcf_proto independent on qdisc
Patch 34 finally enables block sharing for cls_ingress and cls_clsact
Iproute2 implementation is here:
https://github.com/jpirko/iproute2_mlxsw/commit/f91ff81e3b307adfe769f29b3c04c70f39a2520c
The next patchset will introduce block sharing for mlxsw. For the
curious ones the patches could be found here:
https://github.com/jpirko/linux_mlxsw/commits/jiri_devel_shblock
Jiri Pirko (34):
net: sched: store Qdisc pointer in struct block
net: sched: introduce support for multiple filter chain pointers
registration
net: sched: introduce shared filter blocks infrastructure
net: sched: teach tcf_bind/unbind_filter to use block->q
net: sched: ematch: obtain net pointer from blocks
net: core: use dev->ingress_queue instead of tp->q
net: sched: cls_u32: use block instead of q in tc_u_common
net: sched: avoid usage of tp->q in tcf_classify
net: sched: tcindex, fw, flow: use tcf_block_q helper to get struct
Qdisc
net: sched: use tcf_block_q helper to get q pointer for sch_tree_lock
net: sched: propagate q and parent from caller down to tcf_fill_node
net: sched: add block bind/unbind notification to drivers
net: sched: introduce per-block callbacks
net: sched: use extended variants of block get and put in ingress and
clsact qdiscs
net: sched: use tc_setup_cb_call to call per-block callbacks
net: sched: cls_matchall: call block callbacks for offload
net: sched: cls_u32: swap u32_remove_hw_knode and u32_remove_hw_hnode
net: sched: cls_u32: call block callbacks for offload
net: sched: cls_bpf: call block callbacks for offload
mlxsw: spectrum: Convert ndo_setup_tc offloads to block callbacks
mlx5e: Convert ndo_setup_tc offloads to block callbacks
bnxt: Convert ndo_setup_tc offloads to block callbacks
cxgb4: Convert ndo_setup_tc offloads to block callbacks
ixgbe: Convert ndo_setup_tc offloads to block callbacks
mlx5e_rep: Convert ndo_setup_tc offloads to block callbacks
nfp: flower: Convert ndo_setup_tc offloads to block callbacks
nfp: bpf: Convert ndo_setup_tc offloads to block callbacks
dsa: Convert ndo_setup_tc offloads to block callbacks
net: sched: avoid ndo_setup_tc calls for TC_SETUP_CLS*
net: sched: remove unused classid field from tc_cls_common_offload
net: sched: remove unused is_classid_clsact_ingress/egress helpers
net: sched: introduce block mechanism to handle netif_keep_dst calls
net: sched: remove classid and q fields from tcf_proto
net: sched: allow ingress and clsact qdiscs to share filter blocks
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 37 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 3 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 41 +-
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 42 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 45 +-
drivers/net/ethernet/mellanox/mlx5/core/en.h | 4 +-
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 45 +-
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c | 62 ++-
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 83 +++-
drivers/net/ethernet/netronome/nfp/bpf/main.c | 51 +-
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 4 +
.../net/ethernet/netronome/nfp/flower/offload.c | 54 ++-
include/linux/netdevice.h | 1 +
include/net/pkt_cls.h | 195 +++++++-
include/net/pkt_sched.h | 14 +-
include/net/sch_generic.h | 15 +-
include/uapi/linux/pkt_sched.h | 12 +
net/core/dev.c | 21 +-
net/dsa/slave.c | 64 ++-
net/sched/cls_api.c | 527 +++++++++++++++++++--
net/sched/cls_bpf.c | 32 +-
net/sched/cls_flow.c | 9 +-
net/sched/cls_flower.c | 29 +-
net/sched/cls_fw.c | 5 +-
net/sched/cls_matchall.c | 58 +--
net/sched/cls_route.c | 2 +-
net/sched/cls_tcindex.c | 5 +-
net/sched/cls_u32.c | 79 ++-
net/sched/ematch.c | 2 +-
net/sched/sch_api.c | 6 +-
net/sched/sch_atm.c | 4 +-
net/sched/sch_cbq.c | 2 +-
net/sched/sch_drr.c | 2 +-
net/sched/sch_dsmark.c | 2 +-
net/sched/sch_fq_codel.c | 2 +-
net/sched/sch_hfsc.c | 4 +-
net/sched/sch_htb.c | 4 +-
net/sched/sch_ingress.c | 123 ++++-
net/sched/sch_multiq.c | 2 +-
net/sched/sch_prio.c | 2 +-
net/sched/sch_qfq.c | 2 +-
net/sched/sch_sfb.c | 2 +-
net/sched/sch_sfq.c | 2 +-
43 files changed, 1370 insertions(+), 330 deletions(-)
--
2.9.5
^ permalink raw reply
* [patch net-next 01/34] net: sched: store Qdisc pointer in struct block
From: Jiri Pirko @ 2017-10-12 17:17 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
matanb, leonro, idosch, jakub.kicinski, ast, daniel, simon.horman,
pieter.jansenvanvuuren, john.hurley, edumazet, dsahern,
alexander.h.duyck, john.fastabend, willemb
In-Reply-To: <20171012171823.1431-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@mellanox.com>
Prepare for removal of tp->q and store Qdisc pointer in the block
structure.
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/pkt_cls.h | 4 ++--
include/net/sch_generic.h | 1 +
net/sched/cls_api.c | 3 ++-
net/sched/sch_atm.c | 4 ++--
net/sched/sch_cbq.c | 2 +-
net/sched/sch_drr.c | 2 +-
net/sched/sch_dsmark.c | 2 +-
net/sched/sch_fq_codel.c | 2 +-
net/sched/sch_hfsc.c | 4 ++--
net/sched/sch_htb.c | 4 ++--
net/sched/sch_ingress.c | 6 +++---
net/sched/sch_multiq.c | 2 +-
net/sched/sch_prio.c | 2 +-
net/sched/sch_qfq.c | 2 +-
net/sched/sch_sfb.c | 2 +-
net/sched/sch_sfq.c | 2 +-
16 files changed, 23 insertions(+), 21 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index f526374..772dfa8 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -22,7 +22,7 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
bool create);
void tcf_chain_put(struct tcf_chain *chain);
int tcf_block_get(struct tcf_block **p_block,
- struct tcf_proto __rcu **p_filter_chain);
+ struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q);
void tcf_block_put(struct tcf_block *block);
int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
struct tcf_result *res, bool compat_mode);
@@ -30,7 +30,7 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
#else
static inline
int tcf_block_get(struct tcf_block **p_block,
- struct tcf_proto __rcu **p_filter_chain)
+ struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q)
{
return 0;
}
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 684d8ed..df4032c 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -270,6 +270,7 @@ struct tcf_chain {
struct tcf_block {
struct list_head chain_list;
+ struct Qdisc *q;
};
static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2977b8a..f7d3f1f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -241,7 +241,7 @@ tcf_chain_filter_chain_ptr_set(struct tcf_chain *chain,
}
int tcf_block_get(struct tcf_block **p_block,
- struct tcf_proto __rcu **p_filter_chain)
+ struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q)
{
struct tcf_block *block = kzalloc(sizeof(*block), GFP_KERNEL);
struct tcf_chain *chain;
@@ -257,6 +257,7 @@ int tcf_block_get(struct tcf_block **p_block,
goto err_chain_create;
}
tcf_chain_filter_chain_ptr_set(chain, p_filter_chain);
+ block->q = q;
*p_block = block;
return 0;
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index c5fcdf1..2dbd249 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -281,7 +281,7 @@ static int atm_tc_change(struct Qdisc *sch, u32 classid, u32 parent,
goto err_out;
}
- error = tcf_block_get(&flow->block, &flow->filter_list);
+ error = tcf_block_get(&flow->block, &flow->filter_list, sch);
if (error) {
kfree(flow);
goto err_out;
@@ -546,7 +546,7 @@ static int atm_tc_init(struct Qdisc *sch, struct nlattr *opt)
p->link.q = &noop_qdisc;
pr_debug("atm_tc_init: link (%p) qdisc %p\n", &p->link, p->link.q);
- err = tcf_block_get(&p->link.block, &p->link.filter_list);
+ err = tcf_block_get(&p->link.block, &p->link.filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index dcef97f..c3b92d6 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1566,7 +1566,7 @@ cbq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, struct nlattr **t
if (cl == NULL)
goto failure;
- err = tcf_block_get(&cl->block, &cl->filter_list);
+ err = tcf_block_get(&cl->block, &cl->filter_list, sch);
if (err) {
kfree(cl);
return err;
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 2d0e8d4..753dc7a 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -412,7 +412,7 @@ static int drr_init_qdisc(struct Qdisc *sch, struct nlattr *opt)
struct drr_sched *q = qdisc_priv(sch);
int err;
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
err = qdisc_class_hash_init(&q->clhash);
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 2836c80..fb4fb71 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -344,7 +344,7 @@ static int dsmark_init(struct Qdisc *sch, struct nlattr *opt)
if (!opt)
goto errout;
- err = tcf_block_get(&p->block, &p->filter_list);
+ err = tcf_block_get(&p->block, &p->filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index de3b57c..3c40ede 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -481,7 +481,7 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)
return err;
}
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 3f88b75..a692184 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1033,7 +1033,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (cl == NULL)
return -ENOBUFS;
- err = tcf_block_get(&cl->block, &cl->filter_list);
+ err = tcf_block_get(&cl->block, &cl->filter_list, sch);
if (err) {
kfree(cl);
return err;
@@ -1405,7 +1405,7 @@ hfsc_init_qdisc(struct Qdisc *sch, struct nlattr *opt)
return err;
q->eligible = RB_ROOT;
- err = tcf_block_get(&q->root.block, &q->root.filter_list);
+ err = tcf_block_get(&q->root.block, &q->root.filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index c6d7ae8..57be73c 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1030,7 +1030,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt)
if (!opt)
return -EINVAL;
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
@@ -1393,7 +1393,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
if (!cl)
goto failure;
- err = tcf_block_get(&cl->block, &cl->filter_list);
+ err = tcf_block_get(&cl->block, &cl->filter_list, sch);
if (err) {
kfree(cl);
goto failure;
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 44de4ee..9ccc1b8 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -59,7 +59,7 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
struct net_device *dev = qdisc_dev(sch);
int err;
- err = tcf_block_get(&q->block, &dev->ingress_cl_list);
+ err = tcf_block_get(&q->block, &dev->ingress_cl_list, sch);
if (err)
return err;
@@ -153,11 +153,11 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt)
struct net_device *dev = qdisc_dev(sch);
int err;
- err = tcf_block_get(&q->ingress_block, &dev->ingress_cl_list);
+ err = tcf_block_get(&q->ingress_block, &dev->ingress_cl_list, sch);
if (err)
return err;
- err = tcf_block_get(&q->egress_block, &dev->egress_cl_list);
+ err = tcf_block_get(&q->egress_block, &dev->egress_cl_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c
index ff4fc3e..31e0a28 100644
--- a/net/sched/sch_multiq.c
+++ b/net/sched/sch_multiq.c
@@ -245,7 +245,7 @@ static int multiq_init(struct Qdisc *sch, struct nlattr *opt)
if (opt == NULL)
return -EINVAL;
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index 2dd6c68..95fad34 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -212,7 +212,7 @@ static int prio_init(struct Qdisc *sch, struct nlattr *opt)
if (!opt)
return -EINVAL;
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 6ddfd49..8694c7b 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -1419,7 +1419,7 @@ static int qfq_init_qdisc(struct Qdisc *sch, struct nlattr *opt)
int i, j, err;
u32 max_cl_shift, maxbudg_shift, max_classes;
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index cc39e17..487d375 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -553,7 +553,7 @@ static int sfb_init(struct Qdisc *sch, struct nlattr *opt)
struct sfb_sched_data *q = qdisc_priv(sch);
int err;
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 74ea863..123a53a 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -725,7 +725,7 @@ static int sfq_init(struct Qdisc *sch, struct nlattr *opt)
setup_deferrable_timer(&q->perturb_timer, sfq_perturbation,
(unsigned long)sch);
- err = tcf_block_get(&q->block, &q->filter_list);
+ err = tcf_block_get(&q->block, &q->filter_list, sch);
if (err)
return err;
--
2.9.5
^ permalink raw reply related
* [patch net-next 02/34] net: sched: introduce support for multiple filter chain pointers registration
From: Jiri Pirko @ 2017-10-12 17:17 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, jeffrey.t.kirsher, saeedm,
matanb, leonro, idosch, jakub.kicinski, ast, daniel, simon.horman,
pieter.jansenvanvuuren, john.hurley, edumazet, dsahern,
alexander.h.duyck, john.fastabend, willemb
In-Reply-To: <20171012171823.1431-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@mellanox.com>
So far, there was possible only to register a single filter chain
pointer to block->chain[0]. However, when the blocks will get shareable,
we need to allow multiple filter chain pointers registration.
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/pkt_sched.h | 7 +++++
include/net/sch_generic.h | 3 ++-
net/sched/cls_api.c | 68 ++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 67 insertions(+), 11 deletions(-)
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 259bc19..2d234af 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -4,7 +4,9 @@
#include <linux/jiffies.h>
#include <linux/ktime.h>
#include <linux/if_vlan.h>
+#include <linux/netdevice.h>
#include <net/sch_generic.h>
+#include <net/net_namespace.h>
#include <uapi/linux/pkt_sched.h>
#define DEFAULT_TX_QUEUE_LEN 1000
@@ -146,4 +148,9 @@ static inline bool is_classid_clsact_egress(u32 classid)
TC_H_MIN(classid) == TC_H_MIN(TC_H_MIN_EGRESS);
}
+static inline struct net *qdisc_net(struct Qdisc *q)
+{
+ return dev_net(q->dev_queue->dev);
+}
+
#endif
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index df4032c..6583c59 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -261,7 +261,7 @@ struct qdisc_skb_cb {
struct tcf_chain {
struct tcf_proto __rcu *filter_chain;
- struct tcf_proto __rcu **p_filter_chain;
+ struct list_head filter_chain_list;
struct list_head list;
struct tcf_block *block;
u32 index; /* chain index */
@@ -270,6 +270,7 @@ struct tcf_chain {
struct tcf_block {
struct list_head chain_list;
+ struct net *net;
struct Qdisc *q;
};
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index f7d3f1f..0ffd79a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -171,6 +171,11 @@ static void tcf_proto_destroy(struct tcf_proto *tp)
kfree_rcu(tp, rcu);
}
+struct tfc_filter_chain_list_item {
+ struct list_head list;
+ struct tcf_proto __rcu **p_filter_chain;
+};
+
static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
u32 chain_index)
{
@@ -179,6 +184,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
chain = kzalloc(sizeof(*chain), GFP_KERNEL);
if (!chain)
return NULL;
+ INIT_LIST_HEAD(&chain->filter_chain_list);
list_add_tail(&chain->list, &block->chain_list);
chain->block = block;
chain->index = chain_index;
@@ -188,10 +194,11 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
static void tcf_chain_flush(struct tcf_chain *chain)
{
+ struct tfc_filter_chain_list_item *item;
struct tcf_proto *tp;
- if (chain->p_filter_chain)
- RCU_INIT_POINTER(*chain->p_filter_chain, NULL);
+ list_for_each_entry(item, &chain->filter_chain_list, list)
+ RCU_INIT_POINTER(*item->p_filter_chain, NULL);
while ((tp = rtnl_dereference(chain->filter_chain)) != NULL) {
RCU_INIT_POINTER(chain->filter_chain, tp->next);
tcf_chain_put(chain);
@@ -233,11 +240,41 @@ void tcf_chain_put(struct tcf_chain *chain)
}
EXPORT_SYMBOL(tcf_chain_put);
+static int
+tcf_chain_filter_chain_ptr_add(struct tcf_chain *chain,
+ struct tcf_proto __rcu **p_filter_chain)
+{
+ struct tfc_filter_chain_list_item *item;
+
+ item = kmalloc(sizeof(*item), GFP_KERNEL);
+ if (!item)
+ return -ENOMEM;
+ item->p_filter_chain = p_filter_chain;
+ list_add(&item->list, &chain->filter_chain_list);
+ return 0;
+}
+
static void
-tcf_chain_filter_chain_ptr_set(struct tcf_chain *chain,
+tcf_chain_filter_chain_ptr_del(struct tcf_chain *chain,
struct tcf_proto __rcu **p_filter_chain)
{
- chain->p_filter_chain = p_filter_chain;
+ struct tfc_filter_chain_list_item *item;
+
+ list_for_each_entry(item, &chain->filter_chain_list, list) {
+ if (!p_filter_chain ||
+ item->p_filter_chain == p_filter_chain) {
+ RCU_INIT_POINTER(*item->p_filter_chain, NULL);
+ list_del(&item->list);
+ kfree(item);
+ return;
+ }
+ }
+ WARN_ON(1);
+}
+
+static struct tcf_chain *tcf_block_chain_zero(struct tcf_block *block)
+{
+ return list_first_entry(&block->chain_list, struct tcf_chain, list);
}
int tcf_block_get(struct tcf_block **p_block,
@@ -256,7 +293,8 @@ int tcf_block_get(struct tcf_block **p_block,
err = -ENOMEM;
goto err_chain_create;
}
- tcf_chain_filter_chain_ptr_set(chain, p_filter_chain);
+ tcf_chain_filter_chain_ptr_add(chain, p_filter_chain);
+ block->net = qdisc_net(q);
block->q = q;
*p_block = block;
return 0;
@@ -274,6 +312,8 @@ void tcf_block_put(struct tcf_block *block)
if (!block)
return;
+ tcf_chain_filter_chain_ptr_del(tcf_block_chain_zero(block), NULL);
+
/* XXX: Standalone actions are not allowed to jump to any chain, and
* bound actions should be all removed after flushing. However,
* filters are destroyed in RCU callbacks, we have to hold the chains
@@ -371,9 +411,13 @@ static void tcf_chain_tp_insert(struct tcf_chain *chain,
struct tcf_chain_info *chain_info,
struct tcf_proto *tp)
{
- if (chain->p_filter_chain &&
- *chain_info->pprev == chain->filter_chain)
- rcu_assign_pointer(*chain->p_filter_chain, tp);
+ if (*chain_info->pprev == chain->filter_chain) {
+ struct tfc_filter_chain_list_item *item;
+
+ list_for_each_entry(item, &chain->filter_chain_list, list)
+ rcu_assign_pointer(*item->p_filter_chain, tp);
+ }
+
RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain_info));
rcu_assign_pointer(*chain_info->pprev, tp);
tcf_chain_hold(chain);
@@ -385,8 +429,12 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
{
struct tcf_proto *next = rtnl_dereference(chain_info->next);
- if (chain->p_filter_chain && tp == chain->filter_chain)
- RCU_INIT_POINTER(*chain->p_filter_chain, next);
+ if (tp == chain->filter_chain) {
+ struct tfc_filter_chain_list_item *item;
+
+ list_for_each_entry(item, &chain->filter_chain_list, list)
+ RCU_INIT_POINTER(*item->p_filter_chain, next);
+ }
RCU_INIT_POINTER(*chain_info->pprev, next);
tcf_chain_put(chain);
}
--
2.9.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox