* Re: [PATCH 0/6 v3] kvmalloc
From: Michal Hocko @ 2017-01-26 10:32 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <20170126100802.GF6590@dhcp22.suse.cz>
On Thu 26-01-17 11:08:02, Michal Hocko wrote:
> On Thu 26-01-17 10:36:49, Daniel Borkmann wrote:
> > On 01/26/2017 08:43 AM, Michal Hocko wrote:
> > > On Wed 25-01-17 21:16:42, Daniel Borkmann wrote:
> [...]
> > > > I assume that kvzalloc() is still the same from [1], right? If so, then
> > > > it would unfortunately (partially) reintroduce the issue that was fixed.
> > > > If you look above at flags, they're also passed to __vmalloc() to not
> > > > trigger OOM in these situations I've experienced.
> > >
> > > Pushing __GFP_NORETRY to __vmalloc doesn't have the effect you might
> > > think it would. It can still trigger the OOM killer becauset the flags
> > > are no propagated all the way down to all allocations requests (e.g.
> > > page tables). This is the same reason why GFP_NOFS is not supported in
> > > vmalloc.
> >
> > Ok, good to know, is that somewhere clearly documented (like for the
> > case with kmalloc())?
>
> I am afraid that we really suck on this front. I will add something.
So I have folded the following to the patch 1. It is in line with
kvmalloc and hopefully at least tell more than the current code.
---
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d89034a393f2..6c1aa2c68887 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1741,6 +1741,13 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
* Allocate enough pages to cover @size from the page level
* allocator with @gfp_mask flags. Map them into contiguous
* kernel virtual space, using a pagetable protection of @prot.
+ *
+ * Reclaim modifiers in @gfp_mask - __GFP_NORETRY, __GFP_REPEAT
+ * and __GFP_NOFAIL are not supported
+ *
+ * Any use of gfp flags outside of GFP_KERNEL should be consulted
+ * with mm people.
+ *
*/
static void *__vmalloc_node(unsigned long size, unsigned long align,
gfp_t gfp_mask, pgprot_t prot,
--
Michal Hocko
SUSE Labs
^ permalink raw reply related
* Re: [PATCH 0/6 v3] kvmalloc
From: Daniel Borkmann @ 2017-01-26 11:04 UTC (permalink / raw)
To: Michal Hocko
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <20170126103216.GG6590@dhcp22.suse.cz>
On 01/26/2017 11:32 AM, Michal Hocko wrote:
> On Thu 26-01-17 11:08:02, Michal Hocko wrote:
>> On Thu 26-01-17 10:36:49, Daniel Borkmann wrote:
>>> On 01/26/2017 08:43 AM, Michal Hocko wrote:
>>>> On Wed 25-01-17 21:16:42, Daniel Borkmann wrote:
>> [...]
>>>>> I assume that kvzalloc() is still the same from [1], right? If so, then
>>>>> it would unfortunately (partially) reintroduce the issue that was fixed.
>>>>> If you look above at flags, they're also passed to __vmalloc() to not
>>>>> trigger OOM in these situations I've experienced.
>>>>
>>>> Pushing __GFP_NORETRY to __vmalloc doesn't have the effect you might
>>>> think it would. It can still trigger the OOM killer becauset the flags
>>>> are no propagated all the way down to all allocations requests (e.g.
>>>> page tables). This is the same reason why GFP_NOFS is not supported in
>>>> vmalloc.
>>>
>>> Ok, good to know, is that somewhere clearly documented (like for the
>>> case with kmalloc())?
>>
>> I am afraid that we really suck on this front. I will add something.
>
> So I have folded the following to the patch 1. It is in line with
> kvmalloc and hopefully at least tell more than the current code.
> ---
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d89034a393f2..6c1aa2c68887 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1741,6 +1741,13 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> * Allocate enough pages to cover @size from the page level
> * allocator with @gfp_mask flags. Map them into contiguous
> * kernel virtual space, using a pagetable protection of @prot.
> + *
> + * Reclaim modifiers in @gfp_mask - __GFP_NORETRY, __GFP_REPEAT
> + * and __GFP_NOFAIL are not supported
We could probably also mention that __GFP_ZERO in @gfp_mask is
supported, though.
> + * Any use of gfp flags outside of GFP_KERNEL should be consulted
> + * with mm people.
Just a question: should that read 'GFP_KERNEL | __GFP_HIGHMEM' as
that is what vmalloc() resp. vzalloc() and others pass as flags?
> + *
> */
Sounds good otherwise, thanks Michal!
> static void *__vmalloc_node(unsigned long size, unsigned long align,
> gfp_t gfp_mask, pgprot_t prot,
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH v4 net] ravb: unmap descriptors when freeing rings
From: Simon Horman @ 2017-01-26 11:04 UTC (permalink / raw)
To: David Miller, Sergei Shtylyov
Cc: Magnus Damm, netdev, linux-renesas-soc, Kazuya Mizuguchi,
Simon Horman
From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
"swiotlb buffer is full" errors occur after repeated initialisation of a
device - f.e. suspend/resume or ip link set up/down. This is because memory
mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit()
is not released. Resolve this problem by unmapping descriptors when
freeing rings.
Fixes: a780893c89f6 ("ravb: unmap descriptors when freeing rings")
Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
[simon: reworked]
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
--
On inspection it appears that a similar problem is also present in
the SH-Ethernet driver.
v4 [Simon Horman]
* As suggested by Sergei Shtylyov
- Use bool as type for new parameter to ravb_tx_free()
- Clean up comment style
- Only increment tx_bytes for transmitted skbs
v3 [Simon Horman]
* As suggested by Sergei Shtylyov
- consistently use le32_to_cpu(desc->dptr)
- Do not clear desc->ds_cc as it is not used
* Paramatise ravb_tx_free() to allow it to free non-transmitted buffers
v2 [Simon Horman]
* As suggested by Sergei Shtylyov
- Use dma_mapping_error() and rx_desc->ds_cc when unmapping RX descriptors;
this is consistent with the way that they are mapped
- Use ravb_tx_free() to clear TX descriptors
* Reduce scope of new local variable
v1 [Kazuya Mizuguchi]
---
drivers/net/ethernet/renesas/ravb_main.c | 111 ++++++++++++++++++-------------
1 file changed, 63 insertions(+), 48 deletions(-)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 89ac1e3f6175..5d66ac721335 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -179,6 +179,48 @@ static struct mdiobb_ops bb_ops = {
.get_mdio_data = ravb_get_mdio_data,
};
+/* Free TX skb function for AVB-IP */
+static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
+{
+ struct ravb_private *priv = netdev_priv(ndev);
+ struct net_device_stats *stats = &priv->stats[q];
+ struct ravb_tx_desc *desc;
+ int free_num = 0;
+ int entry;
+ u32 size;
+
+ for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) {
+ bool txed;
+
+ entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] *
+ NUM_TX_DESC);
+ desc = &priv->tx_ring[q][entry];
+ txed = desc->die_dt == DT_FEMPTY;
+ if (free_txed_only && !txed)
+ break;
+ /* Descriptor type must be checked before all other reads */
+ dma_rmb();
+ size = le16_to_cpu(desc->ds_tagl) & TX_DS;
+ /* Free the original skb. */
+ if (priv->tx_skb[q][entry / NUM_TX_DESC]) {
+ dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
+ size, DMA_TO_DEVICE);
+ /* Last packet descriptor? */
+ if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) {
+ entry /= NUM_TX_DESC;
+ dev_kfree_skb_any(priv->tx_skb[q][entry]);
+ priv->tx_skb[q][entry] = NULL;
+ stats->tx_packets++;
+ }
+ free_num++;
+ }
+ if (!txed)
+ stats->tx_bytes += size;
+ desc->die_dt = DT_EEMPTY;
+ }
+ return free_num;
+}
+
/* Free skb's and DMA buffers for Ethernet AVB */
static void ravb_ring_free(struct net_device *ndev, int q)
{
@@ -194,19 +236,21 @@ static void ravb_ring_free(struct net_device *ndev, int q)
kfree(priv->rx_skb[q]);
priv->rx_skb[q] = NULL;
- /* Free TX skb ringbuffer */
- if (priv->tx_skb[q]) {
- for (i = 0; i < priv->num_tx_ring[q]; i++)
- dev_kfree_skb(priv->tx_skb[q][i]);
- }
- kfree(priv->tx_skb[q]);
- priv->tx_skb[q] = NULL;
-
/* Free aligned TX buffers */
kfree(priv->tx_align[q]);
priv->tx_align[q] = NULL;
if (priv->rx_ring[q]) {
+ for (i = 0; i < priv->num_rx_ring[q]; i++) {
+ struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
+
+ if (!dma_mapping_error(ndev->dev.parent,
+ le32_to_cpu(desc->dptr)))
+ dma_unmap_single(ndev->dev.parent,
+ le32_to_cpu(desc->dptr),
+ PKT_BUF_SZ,
+ DMA_FROM_DEVICE);
+ }
ring_size = sizeof(struct ravb_ex_rx_desc) *
(priv->num_rx_ring[q] + 1);
dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
@@ -215,12 +259,20 @@ static void ravb_ring_free(struct net_device *ndev, int q)
}
if (priv->tx_ring[q]) {
+ ravb_tx_free(ndev, q, false);
+
ring_size = sizeof(struct ravb_tx_desc) *
(priv->num_tx_ring[q] * NUM_TX_DESC + 1);
dma_free_coherent(ndev->dev.parent, ring_size, priv->tx_ring[q],
priv->tx_desc_dma[q]);
priv->tx_ring[q] = NULL;
}
+
+ /* Free TX skb ringbuffer.
+ * SKBs are freed by ravb_tx_free() call above.
+ */
+ kfree(priv->tx_skb[q]);
+ priv->tx_skb[q] = NULL;
}
/* Format skb and descriptor buffer for Ethernet AVB */
@@ -431,44 +483,6 @@ static int ravb_dmac_init(struct net_device *ndev)
return 0;
}
-/* Free TX skb function for AVB-IP */
-static int ravb_tx_free(struct net_device *ndev, int q)
-{
- struct ravb_private *priv = netdev_priv(ndev);
- struct net_device_stats *stats = &priv->stats[q];
- struct ravb_tx_desc *desc;
- int free_num = 0;
- int entry;
- u32 size;
-
- for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) {
- entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] *
- NUM_TX_DESC);
- desc = &priv->tx_ring[q][entry];
- if (desc->die_dt != DT_FEMPTY)
- break;
- /* Descriptor type must be checked before all other reads */
- dma_rmb();
- size = le16_to_cpu(desc->ds_tagl) & TX_DS;
- /* Free the original skb. */
- if (priv->tx_skb[q][entry / NUM_TX_DESC]) {
- dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
- size, DMA_TO_DEVICE);
- /* Last packet descriptor? */
- if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) {
- entry /= NUM_TX_DESC;
- dev_kfree_skb_any(priv->tx_skb[q][entry]);
- priv->tx_skb[q][entry] = NULL;
- stats->tx_packets++;
- }
- free_num++;
- }
- stats->tx_bytes += size;
- desc->die_dt = DT_EEMPTY;
- }
- return free_num;
-}
-
static void ravb_get_tx_tstamp(struct net_device *ndev)
{
struct ravb_private *priv = netdev_priv(ndev);
@@ -902,7 +916,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
spin_lock_irqsave(&priv->lock, flags);
/* Clear TX interrupt */
ravb_write(ndev, ~mask, TIS);
- ravb_tx_free(ndev, q);
+ ravb_tx_free(ndev, q, true);
netif_wake_subqueue(ndev, q);
mmiowb();
spin_unlock_irqrestore(&priv->lock, flags);
@@ -1567,7 +1581,8 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
priv->cur_tx[q] += NUM_TX_DESC;
if (priv->cur_tx[q] - priv->dirty_tx[q] >
- (priv->num_tx_ring[q] - 1) * NUM_TX_DESC && !ravb_tx_free(ndev, q))
+ (priv->num_tx_ring[q] - 1) * NUM_TX_DESC &&
+ !ravb_tx_free(ndev, q, true))
netif_stop_subqueue(ndev, q);
exit:
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
* Re: [PATCH 0/5] GTP: Fine-tuning for some function implementations
From: Andreas Schultz @ 2017-01-26 11:14 UTC (permalink / raw)
To: SF Markus Elfring
Cc: netdev, Alexey Dobriyan, David S. Miller, laforge, Johannes Berg,
pablo, LKML, kernel-janitors
In-Reply-To: <698070a4-e08e-78e5-0214-df69783cce22@users.sourceforge.net>
Hi Markus,
----- On Jan 26, 2017, at 11:15 AM, SF Markus Elfring elfring@users.sourceforge.net wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Jan 2017 11:10:01 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5):
> Use kmalloc_array() in gtp_hashtable_new()
> Improve another size determination in ipv4_pdp_add()
> Adjust 12 checks for null pointers
> Rename jump labels in gtp_encap_enable()
> Rename jump labels in gtp_hashtable_new()
Looks good to me, for all the above:
Reviewed-by: Andreas Schultz <aschultz@tpip.net>
Andreas
>
> drivers/net/gtp.c | 46 ++++++++++++++++++++++++----------------------
> 1 file changed, 24 insertions(+), 22 deletions(-)
>
> --
> 2.11.0
^ permalink raw reply
* Re: [patch net-next] smc: some potential use after free bugs
From: Ursula Braun @ 2017-01-26 11:23 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David S. Miller, linux-s390, netdev, kernel-janitors
In-Reply-To: <20170126090527.GA966@mwanda>
On 01/26/2017 10:05 AM, Dan Carpenter wrote:
> Say we got really unlucky and these failed on the last iteration, then
> it could lead to a use after free bug.
thanks for reporting this! I had already a similar patch prepared, but not
yet submitted. It contains all your added lines plus these additional
pre-initializations at definition time:
@@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
struct smc_connection *conn = &smc->conn;
struct smc_link_group *lgr = conn->lgr;
int tmp_bufsize, tmp_bufsize_short;
- struct smc_buf_desc *sndbuf_desc;
+ struct smc_buf_desc *sndbuf_desc = NULL;
int rc;
/* use socket send buffer size (w/o overhead) as start value */
@@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)
struct smc_connection *conn = &smc->conn;
struct smc_link_group *lgr = conn->lgr;
int tmp_bufsize, tmp_bufsize_short;
- struct smc_buf_desc *rmb_desc;
+ struct smc_buf_desc *rmb_desc = NULL;
int rc;
/* use socket recv buffer size (w/o overhead) as start value */
If you do not contradict, I will post my enhanced patch version.
>
> Fixes: cd6851f30386 ("smc: remote memory buffers (RMBs)")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index 8b1d34378829..941279e1504e 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -535,6 +535,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
> /* if send buffer allocation has failed,
> * try a smaller one
> */
> + sndbuf_desc = NULL;
> continue;
> }
> rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,
> @@ -543,6 +544,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
> if (rc) {
> kfree(sndbuf_desc->cpu_addr);
> kfree(sndbuf_desc);
> + sndbuf_desc = NULL;
> continue; /* if mapping failed, try smaller one */
> }
> sndbuf_desc->used = 1;
> @@ -599,6 +601,7 @@ int smc_rmb_create(struct smc_sock *smc)
> /* if RMB allocation has failed,
> * try a smaller one
> */
> + rmb_desc = NULL;
> continue;
> }
> rc = smc_ib_buf_map(lgr->lnk[SMC_SINGLE_LINK].smcibdev,
> @@ -607,6 +610,7 @@ int smc_rmb_create(struct smc_sock *smc)
> if (rc) {
> kfree(rmb_desc->cpu_addr);
> kfree(rmb_desc);
> + rmb_desc = NULL;
> continue; /* if mapping failed, try smaller one */
> }
> rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,
> @@ -619,6 +623,7 @@ int smc_rmb_create(struct smc_sock *smc)
> DMA_FROM_DEVICE);
> kfree(rmb_desc->cpu_addr);
> kfree(rmb_desc);
> + rmb_desc = NULL;
> continue;
> }
> rmb_desc->used = 1;
>
^ permalink raw reply
* Re: [patch net-next] smc: some potential use after free bugs
From: Dan Carpenter @ 2017-01-26 11:32 UTC (permalink / raw)
To: Ursula Braun; +Cc: David S. Miller, linux-s390, netdev, kernel-janitors
In-Reply-To: <d66e8e35-0be7-ea04-6e1e-7c214cc6c50b@linux.vnet.ibm.com>
On Thu, Jan 26, 2017 at 12:23:02PM +0100, Ursula Braun wrote:
>
> On 01/26/2017 10:05 AM, Dan Carpenter wrote:
> > Say we got really unlucky and these failed on the last iteration, then
> > it could lead to a use after free bug.
> thanks for reporting this! I had already a similar patch prepared, but not
> yet submitted. It contains all your added lines plus these additional
> pre-initializations at definition time:
>
> @@ -509,7 +509,7 @@ int smc_sndbuf_create(struct smc_sock *smc)
> struct smc_connection *conn = &smc->conn;
> struct smc_link_group *lgr = conn->lgr;
> int tmp_bufsize, tmp_bufsize_short;
> - struct smc_buf_desc *sndbuf_desc;
> + struct smc_buf_desc *sndbuf_desc = NULL;
> int rc;
>
> /* use socket send buffer size (w/o overhead) as start value */
>
> @@ -573,7 +575,7 @@ int smc_rmb_create(struct smc_sock *smc)
> struct smc_connection *conn = &smc->conn;
> struct smc_link_group *lgr = conn->lgr;
> int tmp_bufsize, tmp_bufsize_short;
> - struct smc_buf_desc *rmb_desc;
> + struct smc_buf_desc *rmb_desc = NULL;
> int rc;
>
> /* use socket recv buffer size (w/o overhead) as start value */
>
> If you do not contradict, I will post my enhanced patch version.
Those are obviously harmless changes...
My static checker knows those aren't required so it doesn't complain
about uninitialized variables. But not all static analysis tools are as
clever. ;)
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 0/6 v3] kvmalloc
From: Daniel Borkmann @ 2017-01-26 11:33 UTC (permalink / raw)
To: Michal Hocko
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <20170126100802.GF6590@dhcp22.suse.cz>
On 01/26/2017 11:08 AM, Michal Hocko wrote:
> On Thu 26-01-17 10:36:49, Daniel Borkmann wrote:
>> On 01/26/2017 08:43 AM, Michal Hocko wrote:
>>> On Wed 25-01-17 21:16:42, Daniel Borkmann wrote:
> [...]
>>>> I assume that kvzalloc() is still the same from [1], right? If so, then
>>>> it would unfortunately (partially) reintroduce the issue that was fixed.
>>>> If you look above at flags, they're also passed to __vmalloc() to not
>>>> trigger OOM in these situations I've experienced.
>>>
>>> Pushing __GFP_NORETRY to __vmalloc doesn't have the effect you might
>>> think it would. It can still trigger the OOM killer becauset the flags
>>> are no propagated all the way down to all allocations requests (e.g.
>>> page tables). This is the same reason why GFP_NOFS is not supported in
>>> vmalloc.
>>
>> Ok, good to know, is that somewhere clearly documented (like for the
>> case with kmalloc())?
>
> I am afraid that we really suck on this front. I will add something.
Thanks for doing that, much appreciated!
>> If not, could we do that for non-mm folks, or
>> at least add a similar WARN_ON_ONCE() as you did for kvmalloc() to make
>> it obvious to users that a given flag combination is not supported all
>> the way down?
>
> I am not sure that triggering a warning that somebody has used
> __GFP_NOWARN is very helpful ;). I also do not think that covering all the
> supported flags is really feasible. Most of them will not have bad side
> effects. I have added the warning because this API is new and I wanted
> to catch new abusers. Old ones would have to die slowly.
Okay, makes sense then. Just the kdoc comment from your other
mail should help fine already.
>>>> This is effectively the
>>>> same requirement as in other networking areas f.e. that 5bad87348c70
>>>> ("netfilter: x_tables: avoid warn and OOM killer on vmalloc call") has.
>>>> In your comment in kvzalloc() you eventually say that some of the above
>>>> modifiers are not supported. So there would be two options, i) just leave
>>>> out the kvzalloc() chunk for BPF area to avoid the merge conflict and tackle
>>>> it later (along with similar code from 5bad87348c70), or ii) implement
>>>> support for these modifiers as well to your original set. I guess it's not
>>>> too urgent, so we could also proceed with i) if that is easier for you to
>>>> proceed (I don't mind either way).
>>>
>>> Could you clarify why the oom killer in vmalloc matters actually?
>>
>> For both mentioned commits, (privileged) user space can potentially
>> create large allocation requests, where we thus switch to vmalloc()
>> flavor eventually and then OOM starts killing processes to try to
>> satisfy the allocation request. This is bad, because we want the
>> request to just fail instead as it's non-critical and f.e. not kill
>> ssh connection et al. Failing is totally fine in this case, whereas
>> triggering OOM is not.
>
> I see your intention but does it really make any real difference?
> Consider you would back off right before you would have OOMed. Any
> parallel request would just hit the OOM for you. You are (almost) never
> doing an allocation in an isolation.
>
>> In my testing, __GFP_NORETRY did satisfy this
>> just fine, but as you say it seems it's not enough.
>
> Yeah, ptes have been most probably popullated already.
>
>> Given there are
>> multiple places like these in the kernel, could we instead add an
>> option such as __GFP_NOOOM, or just make __GFP_NORETRY supported?
>
> As said above I do not really think that suppressing the OOM killer
> makes any difference because it might be just somebody else doing that
> for you. Also the OOM killer is the MM internal implementation "detail"
> users shouldn't really care. I agree that callers should have a way to
> say they do not want to try really hard and that is not that simple
> for vmalloc unfortunatelly. The main problem here is that gfp mask
> propagation is not that easy to fix without a lot of code churn as some
> of those hardcoded allocation requests are deep in call chains.
I see, that's unfortunate. I understand that there are requests
in parallel and that we might end up with OOM eventually if we're
unlucky, but having some way to tell vmalloc to just not try as
hard as usual would be nice.
> I know this sucks and it would be great to support __GFP_NORETRY to
> [k]vmalloc and maybe we will get there eventually. But for the mean time
> I really think that using kvmalloc wherever possible is much better than
> open coded variants whith expectations which do not hold sometimes.
I totally agree with you that having kvmalloc() as helper is awesome
and probably long overdue as well. :)
> If you disagree I can drop the bpf part of course...
If we could consolidate these spots with kvmalloc() eventually, I'm
all for it. But even if __GFP_NORETRY is not covered down to all
possible paths, it kind of does have an effect already of saying
'don't try too hard', so would it be harmful to still keep that for
now? If it's not, I'd personally prefer to just leave it as is until
there's some form of support by kvmalloc() and friends.
Thanks for your input, Michal!
Cheers,
Daniel
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* RE: [PATCH net 1/3] qed: Release CQ resource under lock on failure
From: Amrani, Ram @ 2017-01-26 11:43 UTC (permalink / raw)
To: Yuval Shaia, Mintz, Yuval
Cc: davem@davemloft.net, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Kalderon, Michal
In-Reply-To: <20170125072426.GA4973@yuval-lap>
> Minor suggestion.
> Can you consider embedding the lock and unlock inside qed_bmap_release_id?
> There are two places where this is bad idea as driver needs to release two
> IDs but still one is in error flow and second is when destroying QP so for
> most cases code may look a bit better.
>
The bitmap functions are being used by various bitmaps one of which is the cid.
The cid code must allocate two consecutive cids. So here the lock is over two calls
to the bit allocating call.
I am planning to recode the cid to use one bit instead of two, this is further
down the list.
Thanks for the suggestion,
Ram
^ permalink raw reply
* Re: [PATCH 0/6 v3] kvmalloc
From: Michal Hocko @ 2017-01-26 11:49 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <5889D7AD.5030103@iogearbox.net>
On Thu 26-01-17 12:04:13, Daniel Borkmann wrote:
> On 01/26/2017 11:32 AM, Michal Hocko wrote:
> > On Thu 26-01-17 11:08:02, Michal Hocko wrote:
> > > On Thu 26-01-17 10:36:49, Daniel Borkmann wrote:
> > > > On 01/26/2017 08:43 AM, Michal Hocko wrote:
> > > > > On Wed 25-01-17 21:16:42, Daniel Borkmann wrote:
> > > [...]
> > > > > > I assume that kvzalloc() is still the same from [1], right? If so, then
> > > > > > it would unfortunately (partially) reintroduce the issue that was fixed.
> > > > > > If you look above at flags, they're also passed to __vmalloc() to not
> > > > > > trigger OOM in these situations I've experienced.
> > > > >
> > > > > Pushing __GFP_NORETRY to __vmalloc doesn't have the effect you might
> > > > > think it would. It can still trigger the OOM killer becauset the flags
> > > > > are no propagated all the way down to all allocations requests (e.g.
> > > > > page tables). This is the same reason why GFP_NOFS is not supported in
> > > > > vmalloc.
> > > >
> > > > Ok, good to know, is that somewhere clearly documented (like for the
> > > > case with kmalloc())?
> > >
> > > I am afraid that we really suck on this front. I will add something.
> >
> > So I have folded the following to the patch 1. It is in line with
> > kvmalloc and hopefully at least tell more than the current code.
> > ---
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index d89034a393f2..6c1aa2c68887 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -1741,6 +1741,13 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> > * Allocate enough pages to cover @size from the page level
> > * allocator with @gfp_mask flags. Map them into contiguous
> > * kernel virtual space, using a pagetable protection of @prot.
> > + *
> > + * Reclaim modifiers in @gfp_mask - __GFP_NORETRY, __GFP_REPEAT
> > + * and __GFP_NOFAIL are not supported
>
> We could probably also mention that __GFP_ZERO in @gfp_mask is
> supported, though.
There are others which would be supported so I would rather stay with
explicit unsupported.
>
> > + * Any use of gfp flags outside of GFP_KERNEL should be consulted
> > + * with mm people.
>
> Just a question: should that read 'GFP_KERNEL | __GFP_HIGHMEM' as
> that is what vmalloc() resp. vzalloc() and others pass as flags?
yes, even though I think that specifying __GFP_HIGHMEM shouldn't be
really necessary. Are there any users who would really insist on vmalloc
pages in lowmem? Anyway this made me recheck kvmalloc_node
implementation and I am not adding this flags which would mean a
regression from the current state. Will fix it up.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 0/6 v3] kvmalloc
From: Michal Hocko @ 2017-01-26 11:58 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <5889DEA3.7040106@iogearbox.net>
On Thu 26-01-17 12:33:55, Daniel Borkmann wrote:
> On 01/26/2017 11:08 AM, Michal Hocko wrote:
[...]
> > If you disagree I can drop the bpf part of course...
>
> If we could consolidate these spots with kvmalloc() eventually, I'm
> all for it. But even if __GFP_NORETRY is not covered down to all
> possible paths, it kind of does have an effect already of saying
> 'don't try too hard', so would it be harmful to still keep that for
> now? If it's not, I'd personally prefer to just leave it as is until
> there's some form of support by kvmalloc() and friends.
Well, you can use kvmalloc(size, GFP_KERNEL|__GFP_NORETRY). It is not
disallowed. It is not _supported_ which means that if it doesn't work as
you expect you are on your own. Which is actually the situation right
now as well. But I still think that this is just not right thing to do.
Even though it might happen to work in some cases it gives a false
impression of a solution. So I would rather go with
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 8697f43cf93c..a6dc4d596f14 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -53,6 +53,11 @@ void bpf_register_map_type(struct bpf_map_type_list *tl)
void *bpf_map_area_alloc(size_t size)
{
+ /*
+ * FIXME: we would really like to not trigger the OOM killer and rather
+ * fail instead. This is not supported right now. Please nag MM people
+ * if these OOM start bothering people.
+ */
return kvzalloc(size, GFP_USER);
}
--
Michal Hocko
SUSE Labs
^ permalink raw reply related
* Re: [PATCH net-next 2/2] net: phy: Marvell: Add mv88e6390 internal PHY
From: Gregory CLEMENT @ 2017-01-26 12:01 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev, Vivien Didelot
In-Reply-To: <1485309314-23942-3-git-send-email-andrew@lunn.ch>
Hi Andrew,
On mer., janv. 25 2017, Andrew Lunn <andrew@lunn.ch> wrote:
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index b5b73ff4329a..9debe1a4a6f6 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -2140,6 +2140,25 @@ static struct phy_driver marvell_drivers[] = {
> .get_strings = marvell_get_strings,
> .get_stats = marvell_get_stats,
> },
> + {
> + .phy_id = MARVELL_PHY_ID_88E6390,
> + .phy_id_mask = MARVELL_PHY_ID_MASK,
> + .name = "Marvell 88E6390",
> + .features = PHY_GBIT_FEATURES,
> + .flags = PHY_HAS_INTERRUPT,
> + .probe = marvell_probe,
In order to get the temperature support I also had to modify the line
above by using m88e1510_probe instead of the marvell_probe. Indeed the
hwmon register was not done marvell_probe.
I think that you use marvell_probe because currently the value you get
are wrong for 6390. So either we have the 6390 which returns an
incorrect value for the temperature until you find the correct setting
or we introduce a new phy_id for the 6341 family with the hwmon support.
Gregory
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Workaround missing PHY ID on mv88e6390
From: Gregory CLEMENT @ 2017-01-26 12:06 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev, Vivien Didelot
In-Reply-To: <1485309314-23942-2-git-send-email-andrew@lunn.ch>
Hi Andrew,
> - return mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
> + err = mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
> + if (err)
> + return err;
> +
> + if (reg == MII_PHYSID2) {
> + /* The mv88e6390 internal PHYS don't have a model number.
> + * Use the switch family model number instead.
> + */
> + if (!(*val & 0x3ff)) {
> + if (chip->info->family == MV88E6XXX_FAMILY_6390)
I needed to test the MV88E6XXX_FAMILY_6341 flag too. But this one have
to be done in my series because before it the flag is not introduced.
As pointed in the other patch, we still need to decide to use the sale
PHY ID or to use a new one.
Thanks,
Gregory
> + *val |= PORT_SWITCH_ID_PROD_NUM_6390;
> + }
> + }
> +
> + return 0;
> }
>
> int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip,
> --
> 2.11.0
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 0/6 v3] kvmalloc
From: Joe Perches @ 2017-01-26 12:14 UTC (permalink / raw)
To: Michal Hocko, Daniel Borkmann
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <20170126103216.GG6590@dhcp22.suse.cz>
On Thu, 2017-01-26 at 11:32 +0100, Michal Hocko wrote:
> So I have folded the following to the patch 1. It is in line with
> kvmalloc and hopefully at least tell more than the current code.
[]
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
[]
> @@ -1741,6 +1741,13 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> * Allocate enough pages to cover @size from the page level
> * allocator with @gfp_mask flags. Map them into contiguous
> * kernel virtual space, using a pagetable protection of @prot.
> + *
> + * Reclaim modifiers in @gfp_mask - __GFP_NORETRY, __GFP_REPEAT
> + * and __GFP_NOFAIL are not supported
Maybe add a BUILD_BUG or a WARN_ON_ONCE to catch new occurrences?
^ permalink raw reply
* Re: [PATCH 0/6 v3] kvmalloc
From: Michal Hocko @ 2017-01-26 12:27 UTC (permalink / raw)
To: Joe Perches
Cc: Daniel Borkmann, Alexei Starovoitov, Andrew Morton,
Vlastimil Babka, Mel Gorman, Johannes Weiner, linux-mm, LKML,
netdev@vger.kernel.org, marcelo.leitner
In-Reply-To: <1485432877.12563.100.camel@perches.com>
On Thu 26-01-17 04:14:37, Joe Perches wrote:
> On Thu, 2017-01-26 at 11:32 +0100, Michal Hocko wrote:
> > So I have folded the following to the patch 1. It is in line with
> > kvmalloc and hopefully at least tell more than the current code.
> []
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> []
> > @@ -1741,6 +1741,13 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> > * Allocate enough pages to cover @size from the page level
> > * allocator with @gfp_mask flags. Map them into contiguous
> > * kernel virtual space, using a pagetable protection of @prot.
> > + *
> > + * Reclaim modifiers in @gfp_mask - __GFP_NORETRY, __GFP_REPEAT
> > + * and __GFP_NOFAIL are not supported
>
> Maybe add a BUILD_BUG or a WARN_ON_ONCE to catch new occurrences?
I would really like to not touch vmalloc in this series.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH iproute2 v5 0/4] update ifstat for new stats
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
To: netdev
Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
idosch, yotamg, ogerlitz, Nogah Frankel
Previously stats were gotten by RTM_GETLINK which returns 32 bits based
statistics. It supports only one type of stats.
Lately, a new method to get stats was added - RTM_GETSTATS. It supports
ability to choose stats type. The basic stats were changed from 32 bits
based to 64 bits based.
This patchset adds ifstat the ability to get extended stats (xstats) by
this method. Its adds xstat of cpu_hits - for packets that hit cpu.
---
v4->v5:
- remove patch 3/4 that added 64 based "normal" stats
- add a new patch 4/4 that updates ifstat man page
v3->v4:
- patch 2/4:
- change xstat name read to avoid redundant copy.
- delete extra line
- patch 4/4:
- change xstat name.
v2->v3:
- patch 1/4:
- add a new patch to reorder includes in misc/ifstat.c
- patch 2/4: (previously 1/3)
- fix typos.
- change error print to use fprintf.
v1->v2:
- change from using RTM_GETSTATS always to using it only for extended
stats.
- Add 64bits extended stats type.
Nogah Frankel (4):
ifstat: Includes reorder
ifstat: Add extended statistics to ifstat
ifstat: Add "sw only" extended statistics to ifstat
ifstat: Add xstat to ifstat man page
man/man8/Makefile | 3 +-
man/man8/ifstat.8 | 12 +++-
misc/ifstat.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++------
3 files changed, 163 insertions(+), 20 deletions(-)
--
2.4.3
^ permalink raw reply
* [PATCH iproute2 v5 1/4] ifstat: Includes reorder
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
To: netdev
Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
idosch, yotamg, ogerlitz, Nogah Frankel
In-Reply-To: <1485434681-33947-1-git-send-email-nogahf@mellanox.com>
Reorder the includes in misc/ifstat.c to match convention.
Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
misc/ifstat.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/misc/ifstat.c b/misc/ifstat.c
index 92d67b0..5bcbcc8 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -28,12 +28,12 @@
#include <math.h>
#include <getopt.h>
-#include <libnetlink.h>
-#include <json_writer.h>
#include <linux/if.h>
#include <linux/if_link.h>
-#include <SNAPSHOT.h>
+#include "libnetlink.h"
+#include "json_writer.h"
+#include "SNAPSHOT.h"
int dump_zeros;
int reset_history;
--
2.4.3
^ permalink raw reply related
* [PATCH iproute2 v5 2/4] ifstat: Add extended statistics to ifstat
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
To: netdev
Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
idosch, yotamg, ogerlitz, Nogah Frankel
In-Reply-To: <1485434681-33947-1-git-send-email-nogahf@mellanox.com>
Extended stats are part of the RTM_GETSTATS method. This patch adds them
to ifstat.
While extended stats can come in many forms, we support only the
rtnl_link_stats64 struct for them (which is the 64 bits version of struct
rtnl_link_stats).
We support stats in the main nesting level, or one lower.
The extension can be called by its name or any shorten of it. If there is
more than one matched, the first one will be picked.
To get the extended stats the flag -x <stats type> is used.
Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
misc/ifstat.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 145 insertions(+), 15 deletions(-)
diff --git a/misc/ifstat.c b/misc/ifstat.c
index 5bcbcc8..9467119 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -34,6 +34,7 @@
#include "libnetlink.h"
#include "json_writer.h"
#include "SNAPSHOT.h"
+#include "utils.h"
int dump_zeros;
int reset_history;
@@ -48,17 +49,21 @@ int pretty;
double W;
char **patterns;
int npatterns;
+bool is_extended;
+int filter_type;
+int sub_type;
char info_source[128];
int source_mismatch;
#define MAXS (sizeof(struct rtnl_link_stats)/sizeof(__u32))
+#define NO_SUB_TYPE 0xffff
struct ifstat_ent {
struct ifstat_ent *next;
char *name;
int ifindex;
- unsigned long long val[MAXS];
+ __u64 val[MAXS];
double rate[MAXS];
__u32 ival[MAXS];
};
@@ -106,6 +111,48 @@ static int match(const char *id)
return 0;
}
+static int get_nlmsg_extended(const struct sockaddr_nl *who,
+ struct nlmsghdr *m, void *arg)
+{
+ struct if_stats_msg *ifsm = NLMSG_DATA(m);
+ struct rtattr *tb[IFLA_STATS_MAX+1];
+ int len = m->nlmsg_len;
+ struct ifstat_ent *n;
+
+ if (m->nlmsg_type != RTM_NEWSTATS)
+ return 0;
+
+ len -= NLMSG_LENGTH(sizeof(*ifsm));
+ if (len < 0)
+ return -1;
+
+ parse_rtattr(tb, IFLA_STATS_MAX, IFLA_STATS_RTA(ifsm), len);
+ if (tb[filter_type] == NULL)
+ return 0;
+
+ n = malloc(sizeof(*n));
+ if (!n)
+ abort();
+
+ n->ifindex = ifsm->ifindex;
+ n->name = strdup(ll_index_to_name(ifsm->ifindex));
+
+ if (sub_type == NO_SUB_TYPE) {
+ memcpy(&n->val, RTA_DATA(tb[filter_type]), sizeof(n->val));
+ } else {
+ struct rtattr *attr;
+
+ attr = parse_rtattr_one_nested(sub_type, tb[filter_type]);
+ if (attr == NULL)
+ return 0;
+ memcpy(&n->val, RTA_DATA(attr), sizeof(n->val));
+ }
+ memset(&n->rate, 0, sizeof(n->rate));
+ n->next = kern_db;
+ kern_db = n;
+ return 0;
+}
+
static int get_nlmsg(const struct sockaddr_nl *who,
struct nlmsghdr *m, void *arg)
{
@@ -147,18 +194,34 @@ static void load_info(void)
{
struct ifstat_ent *db, *n;
struct rtnl_handle rth;
+ __u32 filter_mask;
if (rtnl_open(&rth, 0) < 0)
exit(1);
- if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETLINK) < 0) {
- perror("Cannot send dump request");
- exit(1);
- }
+ if (is_extended) {
+ ll_init_map(&rth);
+ filter_mask = IFLA_STATS_FILTER_BIT(filter_type);
+ if (rtnl_wilddump_stats_req_filter(&rth, AF_UNSPEC, RTM_GETSTATS,
+ filter_mask) < 0) {
+ perror("Cannot send dump request");
+ exit(1);
+ }
- if (rtnl_dump_filter(&rth, get_nlmsg, NULL) < 0) {
- fprintf(stderr, "Dump terminated\n");
- exit(1);
+ if (rtnl_dump_filter(&rth, get_nlmsg_extended, NULL) < 0) {
+ fprintf(stderr, "Dump terminated\n");
+ exit(1);
+ }
+ } else {
+ if (rtnl_wilddump_request(&rth, AF_INET, RTM_GETLINK) < 0) {
+ perror("Cannot send dump request");
+ exit(1);
+ }
+
+ if (rtnl_dump_filter(&rth, get_nlmsg, NULL) < 0) {
+ fprintf(stderr, "Dump terminated\n");
+ exit(1);
+ }
}
rtnl_close(&rth);
@@ -553,10 +616,17 @@ static void update_db(int interval)
}
for (i = 0; i < MAXS; i++) {
double sample;
- unsigned long incr = h1->ival[i] - n->ival[i];
+ __u64 incr;
+
+ if (is_extended) {
+ incr = h1->val[i] - n->val[i];
+ n->val[i] = h1->val[i];
+ } else {
+ incr = (__u32) (h1->ival[i] - n->ival[i]);
+ n->val[i] += incr;
+ n->ival[i] = h1->ival[i];
+ }
- n->val[i] += incr;
- n->ival[i] = h1->ival[i];
sample = (double)(incr*1000)/interval;
if (interval >= scan_interval) {
n->rate[i] += W*(sample-n->rate[i]);
@@ -656,6 +726,47 @@ static int verify_forging(int fd)
return -1;
}
+static void xstat_usage(void)
+{
+ fprintf(stderr,
+"Usage: ifstat supported xstats:\n");
+}
+
+struct extended_stats_options_t {
+ char *name;
+ int id;
+ int sub_type;
+};
+
+/* Note: if one xstat name is subset of another, it should be before it in this
+ * list.
+ * Name length must be under 64 chars.
+ */
+static const struct extended_stats_options_t extended_stats_options[] = {
+};
+
+static const char *get_filter_type(const char *name)
+{
+ int name_len;
+ int i;
+
+ name_len = strlen(name);
+ for (i = 0; i < ARRAY_SIZE(extended_stats_options); i++) {
+ const struct extended_stats_options_t *xstat;
+
+ xstat = &extended_stats_options[i];
+ if (strncmp(name, xstat->name, name_len) == 0) {
+ filter_type = xstat->id;
+ sub_type = xstat->sub_type;
+ return xstat->name;
+ }
+ }
+
+ fprintf(stderr, "invalid ifstat extension %s\n", name);
+ xstat_usage();
+ return NULL;
+}
+
static void usage(void) __attribute__((noreturn));
static void usage(void)
@@ -673,7 +784,8 @@ static void usage(void)
" -s, --noupdate don't update history\n"
" -t, --interval=SECS report average over the last SECS\n"
" -V, --version output version information\n"
-" -z, --zeros show entries with zero activity\n");
+" -z, --zeros show entries with zero activity\n"
+" -x, --extended=TYPE show extended stats of TYPE\n");
exit(-1);
}
@@ -691,6 +803,7 @@ static const struct option longopts[] = {
{ "interval", 1, 0, 't' },
{ "version", 0, 0, 'V' },
{ "zeros", 0, 0, 'z' },
+ { "extended", 1, 0, 'x'},
{ 0 }
};
@@ -699,10 +812,12 @@ int main(int argc, char *argv[])
char hist_name[128];
struct sockaddr_un sun;
FILE *hist_fp = NULL;
+ const char *stats_type = NULL;
int ch;
int fd;
- while ((ch = getopt_long(argc, argv, "hjpvVzrnasd:t:e",
+ is_extended = false;
+ while ((ch = getopt_long(argc, argv, "hjpvVzrnasd:t:ex:",
longopts, NULL)) != EOF) {
switch (ch) {
case 'z':
@@ -743,6 +858,10 @@ int main(int argc, char *argv[])
exit(-1);
}
break;
+ case 'x':
+ stats_type = optarg;
+ is_extended = true;
+ break;
case 'v':
case 'V':
printf("ifstat utility, iproute2-ss%s\n", SNAPSHOT);
@@ -757,6 +876,12 @@ int main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ if (stats_type) {
+ stats_type = get_filter_type(stats_type);
+ if (!stats_type)
+ exit(-1);
+ }
+
sun.sun_family = AF_UNIX;
sun.sun_path[0] = 0;
sprintf(sun.sun_path+1, "ifstat%d", getuid());
@@ -795,8 +920,13 @@ int main(int argc, char *argv[])
snprintf(hist_name, sizeof(hist_name),
"%s", getenv("IFSTAT_HISTORY"));
else
- snprintf(hist_name, sizeof(hist_name),
- "%s/.ifstat.u%d", P_tmpdir, getuid());
+ if (!stats_type)
+ snprintf(hist_name, sizeof(hist_name),
+ "%s/.ifstat.u%d", P_tmpdir, getuid());
+ else
+ snprintf(hist_name, sizeof(hist_name),
+ "%s/.%s_ifstat.u%d", P_tmpdir, stats_type,
+ getuid());
if (reset_history)
unlink(hist_name);
--
2.4.3
^ permalink raw reply related
* [PATCH iproute2 v5 3/4] ifstat: Add "sw only" extended statistics to ifstat
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
To: netdev
Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
idosch, yotamg, ogerlitz, Nogah Frankel
In-Reply-To: <1485434681-33947-1-git-send-email-nogahf@mellanox.com>
Add support for extended statistics of SW only type, for counting only the
packets that went via the cpu. (useful for systems with forward
offloading). It reads it from filter type IFLA_STATS_LINK_OFFLOAD_XSTATS
and sub type IFLA_OFFLOAD_XSTATS_CPU_HIT.
It is under the name 'cpu_hits'
(or any shorten of it as 'cpu' or simply 'c')
For example:
ifstat -x c
Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
misc/ifstat.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/misc/ifstat.c b/misc/ifstat.c
index 9467119..a853ee6 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -729,7 +729,8 @@ static int verify_forging(int fd)
static void xstat_usage(void)
{
fprintf(stderr,
-"Usage: ifstat supported xstats:\n");
+"Usage: ifstat supported xstats:\n"
+" cpu_hits Counts only packets that went via the CPU.\n");
}
struct extended_stats_options_t {
@@ -743,6 +744,7 @@ struct extended_stats_options_t {
* Name length must be under 64 chars.
*/
static const struct extended_stats_options_t extended_stats_options[] = {
+ {"cpu_hits", IFLA_STATS_LINK_OFFLOAD_XSTATS, IFLA_OFFLOAD_XSTATS_CPU_HIT},
};
static const char *get_filter_type(const char *name)
--
2.4.3
^ permalink raw reply related
* [PATCH iproute2 v5 4/4] ifstat: Add xstat to ifstat man page
From: Nogah Frankel @ 2017-01-26 12:44 UTC (permalink / raw)
To: netdev
Cc: stephen, roopa, roszenrami, jbenc, sergei.shtylyov, jiri, eladr,
idosch, yotamg, ogerlitz, Nogah Frankel
In-Reply-To: <1485434681-33947-1-git-send-email-nogahf@mellanox.com>
Add documentation about the extended statistics to the ifstat man page.
Add ifstat man age to the man8 Makefile
Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
man/man8/Makefile | 3 ++-
man/man8/ifstat.8 | 12 +++++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/man/man8/Makefile b/man/man8/Makefile
index 77d347c..bc2fc81 100644
--- a/man/man8/Makefile
+++ b/man/man8/Makefile
@@ -18,7 +18,8 @@ MAN8PAGES = $(TARGETS) ip.8 arpd.8 lnstat.8 routel.8 rtacct.8 rtmon.8 rtpr.8 ss.
tc-connmark.8 tc-csum.8 tc-mirred.8 tc-nat.8 tc-pedit.8 tc-police.8 \
tc-simple.8 tc-skbedit.8 tc-vlan.8 tc-xt.8 tc-ife.8 \
tc-tunnel_key.8 \
- devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8 devlink-sb.8
+ devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8 devlink-sb.8 \
+ ifstat.8
all: $(TARGETS)
diff --git a/man/man8/ifstat.8 b/man/man8/ifstat.8
index e49d868..3ba0088 100644
--- a/man/man8/ifstat.8
+++ b/man/man8/ifstat.8
@@ -14,7 +14,8 @@ ifstat \- handy utility to read network interface statistics
The utility keeps records of the previous data displayed in history files and
by default only shows difference between the last and the current call.
Location of the history files defaults to /tmp/.ifstat.u$UID but may be
-overridden with the IFSTAT_HISTORY environment variable.
+overridden with the IFSTAT_HISTORY environment variable. Similarly, the default
+location for xstat (extended stats) is /tmp/.<xstat name>_ifstat.u$UID.
.SH OPTIONS
.TP
.B \-h, \-\-help
@@ -46,6 +47,15 @@ Report average over the last SECS seconds.
.TP
.B \-z, \-\-zeros
Show entries with zero activity.
+.TP
+.B \-x, \-\-extended=TYPE
+Show extended stats of TYPE. Supported types are:
+
+.in +8
+.B cpu_hits
+- Counts only packets that went via the CPU.
+.in -8
+
.SH ENVIRONMENT
.TP
.B IFSTAT_HISTORY
--
2.4.3
^ permalink raw reply related
* ip_rcv_finish() NULL pointer kernel panic
From: Roy Keene @ 2017-01-26 12:28 UTC (permalink / raw)
To: netdev
[Resending to netdev from LKML]
All,
I am experiencing a kernel panic on different (but identical)
hardware in ip_rcv_finish on Linux 4.4.39 (but I can see no changes that
would improve the situation on 4.4.44, or master).
Looking at the disassembly at the last stack frame it looks like we are
calling a NULL function pointer in net/ipv4/ip_input.c:365
Panic:
[ 214.518262] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 214.612199] IP: [< (null)>] (null)
[ 214.672744] PGD 0
[ 214.696887] Oops: 0010 [#1] SMP
[ 214.735697] Modules linked in: br_netfilter(+) tun 8021q bridge stp llc bonding iTCO_wdt iTCO_vendor_support tpm_tis tpm kvm_intel kvm irqbypass sb_edac edac_core ixgbe mdio ipmi_si ipmi_msghandler lpc_ich mfd_core mousedev evdev igb dca procmemro(O) nokeyctl(O) noptrace(O)
[ 215.029240] CPU: 34 PID: 0 Comm: swapper/34 Tainted: G O 4.4.39 #1
[ 215.116720] Hardware name: Cisco Systems Inc UCSC-C220-M3L/UCSC-C220-M3L, BIOS C220M3.2.0.13a.0.0713160937 07/13/16
[ 215.241644] task: ffff882038fb4380 ti: ffff8810392b0000 task.ti: ffff8810392b0000
[ 215.331207] RIP: 0010:[<0000000000000000>] [< (null)>] (null)
[ 215.420877] RSP: 0018:ffff88103fec3880 EFLAGS: 00010286
[ 215.484436] RAX: ffff881011631000 RBX: ffff881011067100 RCX: 0000000000000000
[ 215.569836] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff881011067100
[ 215.655234] RBP: ffff88103fec38a8 R08: 0000000000000008 R09: ffff8810116300a0
[ 215.740629] R10: 0000000000000000 R11: 0000000000000000 R12: ffff881018917dce
[ 215.826030] R13: ffffffff81c9be00 R14: ffffffff81c9be00 R15: ffff881011630078
[ 215.911432] FS: 0000000000000000(0000) GS:ffff88103fec0000(0000) knlGS:0000000000000000
[ 216.008274] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 216.077032] CR2: 0000000000000000 CR3: 0000001011b9d000 CR4: 00000000001406e0
[ 216.162430] Stack:
[ 216.186461] ffffffff8157d7f9 ffff881011067100 ffff881018917dce ffff881011630000
[ 216.275407] ffffffff81c9be00 ffff88103fec3918 ffffffff8157e0db 0000000000000000
[ 216.364352] 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 216.453301] Call Trace:
[ 216.482536] <IRQ>
[ 216.505533] [<ffffffff8157d7f9>] ? ip_rcv_finish+0x99/0x320
[ 216.575442] [<ffffffff8157e0db>] ip_rcv+0x25b/0x370
[ 216.634842] [<ffffffff81540e0b>] __netif_receive_skb_core+0x2cb/0xa20
[ 216.712965] [<ffffffff81541578>] __netif_receive_skb+0x18/0x60
[ 216.783801] [<ffffffff815415e3>] netif_receive_skb_internal+0x23/0x80
[ 216.861921] [<ffffffff8154165c>] netif_receive_skb+0x1c/0x70
[ 216.930686] [<ffffffffa02f6439>] br_handle_frame_finish+0x1b9/0x5b0 [bridge]
[ 217.016091] [<ffffffff81187a00>] ? ___slab_alloc+0x1d0/0x440
[ 217.084849] [<ffffffffa0584074>] br_nf_pre_routing_finish+0x174/0x3d0 [br_netfilter]
[ 217.178568] [<ffffffffa0584c07>] ? br_nf_pre_routing+0x97/0x470 [br_netfilter]
[ 217.266052] [<ffffffffa02f6280>] ? br_handle_local_finish+0x80/0x80 [bridge]
[ 217.351450] [<ffffffffa0584d17>] br_nf_pre_routing+0x1a7/0x470 [br_netfilter]
[ 217.437891] [<ffffffff81572f6d>] nf_iterate+0x5d/0x70
[ 217.499367] [<ffffffff81572fe4>] nf_hook_slow+0x64/0xc0
[ 217.562928] [<ffffffffa02f69e9>] br_handle_frame+0x1b9/0x290 [bridge]
[ 217.641048] [<ffffffffa02f6280>] ? br_handle_local_finish+0x80/0x80 [bridge]
[ 217.726446] [<ffffffff81540e82>] __netif_receive_skb_core+0x342/0xa20
[ 217.804566] [<ffffffff815a7916>] ? tcp4_gro_receive+0x126/0x1d0
[ 217.876445] [<ffffffff815b7446>] ? inet_gro_receive+0x1c6/0x250
[ 217.948322] [<ffffffff81541578>] __netif_receive_skb+0x18/0x60
[ 218.019161] [<ffffffff815415e3>] netif_receive_skb_internal+0x23/0x80
[ 218.097281] [<ffffffff81542213>] napi_gro_receive+0xc3/0x110
[ 218.166051] [<ffffffffa00a801f>] ixgbe_clean_rx_irq+0x52f/0xa70 [ixgbe]
[ 218.246255] [<ffffffffa00a9248>] ixgbe_poll+0x438/0x790 [ixgbe]
[ 218.318131] [<ffffffff81541a6e>] net_rx_action+0x1ee/0x320
[ 218.384813] [<ffffffff8109c837>] ? handle_irq_event_percpu+0x167/0x1d0
[ 218.463973] [<ffffffff8105c3c1>] __do_softirq+0x101/0x280
[ 218.529608] [<ffffffff8105c69e>] irq_exit+0x8e/0x90
[ 218.589007] [<ffffffff816dd504>] do_IRQ+0x54/0xd0
[ 218.646323] [<ffffffff816dba02>] common_interrupt+0x82/0x82
[ 218.714039] <EOI>
[ 218.737040] [<ffffffff814fb4f3>] ? cpuidle_enter_state+0x133/0x2a0
[ 218.814226] [<ffffffff814fb4cf>] ? cpuidle_enter_state+0x10f/0x2a0
[ 218.889224] [<ffffffff814fb697>] cpuidle_enter+0x17/0x20
[ 218.953825] [<ffffffff81093cf1>] cpu_startup_entry+0x2a1/0x300
[ 219.024663] [<ffffffff8103838d>] start_secondary+0xed/0xf0
[ 219.091337] Code: Bad RIP value.
[ 219.131186] RIP [< (null)>] (null)
[ 219.192770] RSP <ffff88103fec3880>
[ 219.234483] CR2: 0000000000000000
[ 219.274121] ---[ end trace 9ce5d4620e3bcbdf ]---
[ 219.274125] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 219.274126] IP: [< (null)>] (null)
[ 219.274126] PGD 0
[ 219.274128] Oops: 0010 [#2] SMP
[ 219.274137] Modules linked in: br_netfilter(+) tun 8021q bridge stp llc bonding iTCO_wdt iTCO_vendor_support tpm_tis tpm kvm_intel kvm irqbypass sb_edac edac_core ixgbe mdio ipmi_si ipmi_msghandler lpc_ich mfd_core mousedev evdev igb dca procmemro(O) nokeyctl(O) noptrace(O)
[ 219.274139] CPU: 7 PID: 0 Comm: swapper/7 Tainted: G D O 4.4.39 #1
[ 219.274140] Hardware name: Cisco Systems Inc UCSC-C220-M3L/UCSC-C220-M3L, BIOS C220M3.2.0.13a.0.0713160937 07/13/16
[ 219.274141] task: ffff882038f84380 ti: ffff881039044000 task.ti: ffff881039044000
[ 219.274142] RIP: 0010:[<0000000000000000>] [< (null)>] (null)
[ 219.274143] RSP: 0018:ffff88103fce3880 EFLAGS: 00010286
[ 219.274143] RAX: ffff881
Notes:
----
include/linux/skbuff.h:
734 #define SKB_DST_NOREF 1UL
735 #define SKB_DST_PTRMASK ~(SKB_DST_NOREF)
...
743 static inline struct dst_entry *skb_dst(const struct sk_buff *skb) {
745 /* If refdst was not refcounted, check we still are in a
746 * rcu_read_lock section
747 */
748 WARN_ON((skb->_skb_refdst & SKB_DST_NOREF) &&
749 !rcu_read_lock_held() &&
750 !rcu_read_lock_bh_held());
751 return (struct dst_entry *)(skb->_skb_refdst & SKB_DST_PTRMASK);
752 }
---
include/net/dst.h:
496 static inline int dst_input(struct sk_buff *skb) {
498 return skb_dst(skb)->input(skb);
499 }
---
net/ipv4/ip_input.c:
359 rt = skb_rtable(skb);
360 if (rt->rt_type == RTN_MULTICAST) {
361 IP_UPD_PO_STATS_BH(net, IPSTATS_MIB_INMCAST, skb->len);
362 } else if (rt->rt_type == RTN_BROADCAST)
363 IP_UPD_PO_STATS_BH(net, IPSTATS_MIB_INBCAST, skb->len);
364
365 return dst_input(skb);
----
Expand net/ipv4/ip_input.c:365:
return dst_input(skb);
Into:
return skb_dst(skb)->input(skb);
Into:
return ((struct dst_entry *)(skb->_skb_refdst & SKB_DST_PTRMASK))->input(skb);
Into:
return ((struct dst_entry *)(skb->_skb_refdst & (~(1UL))))->input(skb);
Into:
return ((struct dst_entry *)(skb->_skb_refdst & 0xfffffffffffffffe))->input(skb);
----
Disassembly with C code next to it
0xffffffff8157d7d4 <ip_rcv_finish+116> movzwl 0xa0(%rdx),%edx | rt = skb_rtable(skb);
0xffffffff8157d7db <ip_rcv_finish+123> cmp $0x5,%dx | if (rt->rt_type == RTN_MULTICAST) {
0xffffffff8157d7df <ip_rcv_finish+127> je 0xffffffff8157d908 <ip_rcv_finish+424> ....
0xffffffff8157d7e5 <ip_rcv_finish+133> cmp $0x3,%dx | } else if (rt->rt_type == RTN_BROADCAST)
0xffffffff8157d7e9 <ip_rcv_finish+137> je 0xffffffff8157d87e <ip_rcv_finish+286> ...
0xffffffff8157d7ef <ip_rcv_finish+143> and $0xfffffffffffffffe,%rax | skb->_skb_refdst & SKB_DST_PTRMASK
0xffffffff8157d7f3 <ip_rcv_finish+147> mov %rbx,%rdi
--> 0xffffffff8157d7f6 <ip_rcv_finish+150> callq *0x50(%rax) | ((struct dst_entry *)(skb->_skb_refdst & SKB_DST_PTRMASK))->input(skb)
0xffffffff8157d7f9 <ip_rcv_finish+153> pop %rbx
0xffffffff8157d7fa <ip_rcv_finish+154> pop %r12
0xffffffff8157d7fc <ip_rcv_finish+156> pop %r13
0xffffffff8157d7fe <ip_rcv_finish+158> pop %r14
0xffffffff8157d800 <ip_rcv_finish+160> pop %rbp
0xffffffff8157d801 <ip_rcv_finish+161> retq
----
This leads me to believe that ((struct dst_entry *)(skb->_skb_refdst & SKB_DST_PTRMASK))->input is NULL and we are jumping there (callq).
Under what conditions would ((struct dst_entry *)(skb->_skb_refdst &
SKB_DST_PTRMASK))->input be NULL at this point ?
Thanks,
Roy Keene
^ permalink raw reply
* Re: [PATCH v4 net] ravb: unmap descriptors when freeing rings
From: Sergei Shtylyov @ 2017-01-26 12:58 UTC (permalink / raw)
To: Simon Horman, David Miller
Cc: Magnus Damm, netdev, linux-renesas-soc, Kazuya Mizuguchi
In-Reply-To: <1485428699-2598-1-git-send-email-horms+renesas@verge.net.au>
On 01/26/2017 02:04 PM, Simon Horman wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> "swiotlb buffer is full" errors occur after repeated initialisation of a
> device - f.e. suspend/resume or ip link set up/down. This is because memory
> mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit()
> is not released. Resolve this problem by unmapping descriptors when
> freeing rings.
>
> Fixes: a780893c89f6 ("ravb: unmap descriptors when freeing rings")
No such SHA1 hash known and the summary is from this patch.
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> [simon: reworked]
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> --
>
> On inspection it appears that a similar problem is also present in
> the SH-Ethernet driver.
>
> v4 [Simon Horman]
> * As suggested by Sergei Shtylyov
> - Use bool as type for new parameter to ravb_tx_free()
> - Clean up comment style
> - Only increment tx_bytes for transmitted skbs
>
> v3 [Simon Horman]
> * As suggested by Sergei Shtylyov
> - consistently use le32_to_cpu(desc->dptr)
> - Do not clear desc->ds_cc as it is not used
> * Paramatise ravb_tx_free() to allow it to free non-transmitted buffers
>
> v2 [Simon Horman]
> * As suggested by Sergei Shtylyov
> - Use dma_mapping_error() and rx_desc->ds_cc when unmapping RX descriptors;
> this is consistent with the way that they are mapped
> - Use ravb_tx_free() to clear TX descriptors
> * Reduce scope of new local variable
>
> v1 [Kazuya Mizuguchi]
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 111 ++++++++++++++++++-------------
> 1 file changed, 63 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 89ac1e3f6175..5d66ac721335 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -179,6 +179,48 @@ static struct mdiobb_ops bb_ops = {
> .get_mdio_data = ravb_get_mdio_data,
> };
>
> +/* Free TX skb function for AVB-IP */
> +static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
> +{
> + struct ravb_private *priv = netdev_priv(ndev);
> + struct net_device_stats *stats = &priv->stats[q];
> + struct ravb_tx_desc *desc;
> + int free_num = 0;
> + int entry;
> + u32 size;
> +
> + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) {
> + bool txed;
> +
> + entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] *
> + NUM_TX_DESC);
> + desc = &priv->tx_ring[q][entry];
> + txed = desc->die_dt == DT_FEMPTY;
> + if (free_txed_only && !txed)
> + break;
> + /* Descriptor type must be checked before all other reads */
> + dma_rmb();
> + size = le16_to_cpu(desc->ds_tagl) & TX_DS;
> + /* Free the original skb. */
> + if (priv->tx_skb[q][entry / NUM_TX_DESC]) {
> + dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> + size, DMA_TO_DEVICE);
> + /* Last packet descriptor? */
> + if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) {
> + entry /= NUM_TX_DESC;
> + dev_kfree_skb_any(priv->tx_skb[q][entry]);
> + priv->tx_skb[q][entry] = NULL;
> + stats->tx_packets++;
We're still incrementing the packet # even when the packet hasn't been sent...
> + }
> + free_num++;
> + }
> + if (!txed)
Your logic is reversed! :-(
> + stats->tx_bytes += size;
> + desc->die_dt = DT_EEMPTY;
> + }
> + return free_num;
> +}
> +
> /* Free skb's and DMA buffers for Ethernet AVB */
> static void ravb_ring_free(struct net_device *ndev, int q)
> {
> @@ -194,19 +236,21 @@ static void ravb_ring_free(struct net_device *ndev, int q)
[...]
>
> if (priv->rx_ring[q]) {
> + for (i = 0; i < priv->num_rx_ring[q]; i++) {
> + struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> +
> + if (!dma_mapping_error(ndev->dev.parent,
> + le32_to_cpu(desc->dptr)))
I'd check for zero buffer length (that marks unmapped RX buffers) but this
should be good too... it's just that the length check is shorter. :-)
> + dma_unmap_single(ndev->dev.parent,
> + le32_to_cpu(desc->dptr),
> + PKT_BUF_SZ,
> + DMA_FROM_DEVICE);
> + }
> ring_size = sizeof(struct ravb_ex_rx_desc) *
> (priv->num_rx_ring[q] + 1);
> dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 0/6 v3] kvmalloc
From: Daniel Borkmann @ 2017-01-26 13:10 UTC (permalink / raw)
To: Michal Hocko
Cc: Alexei Starovoitov, Andrew Morton, Vlastimil Babka, Mel Gorman,
Johannes Weiner, linux-mm, LKML, netdev@vger.kernel.org,
marcelo.leitner
In-Reply-To: <20170126115833.GI6590@dhcp22.suse.cz>
On 01/26/2017 12:58 PM, Michal Hocko wrote:
> On Thu 26-01-17 12:33:55, Daniel Borkmann wrote:
>> On 01/26/2017 11:08 AM, Michal Hocko wrote:
> [...]
>>> If you disagree I can drop the bpf part of course...
>>
>> If we could consolidate these spots with kvmalloc() eventually, I'm
>> all for it. But even if __GFP_NORETRY is not covered down to all
>> possible paths, it kind of does have an effect already of saying
>> 'don't try too hard', so would it be harmful to still keep that for
>> now? If it's not, I'd personally prefer to just leave it as is until
>> there's some form of support by kvmalloc() and friends.
>
> Well, you can use kvmalloc(size, GFP_KERNEL|__GFP_NORETRY). It is not
> disallowed. It is not _supported_ which means that if it doesn't work as
> you expect you are on your own. Which is actually the situation right
> now as well. But I still think that this is just not right thing to do.
> Even though it might happen to work in some cases it gives a false
> impression of a solution. So I would rather go with
Hmm. 'On my own' means, we could potentially BUG somewhere down the
vmalloc implementation, etc, presumably? So it might in-fact be
harmful to pass that, right?
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8697f43cf93c..a6dc4d596f14 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -53,6 +53,11 @@ void bpf_register_map_type(struct bpf_map_type_list *tl)
>
> void *bpf_map_area_alloc(size_t size)
> {
> + /*
> + * FIXME: we would really like to not trigger the OOM killer and rather
> + * fail instead. This is not supported right now. Please nag MM people
> + * if these OOM start bothering people.
> + */
Ok, I know this is out of scope for this series, but since i) this
is _not_ the _only_ spot right now which has such a construct and ii)
I am already kind of nagging a bit ;), my question would be, what
would it take to start supporting it?
> return kvzalloc(size, GFP_USER);
> }
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 2/5] gtp: Improve another size determination in ipv4_pdp_add()
From: Alexey Dobriyan @ 2017-01-26 13:11 UTC (permalink / raw)
To: SF Markus Elfring
Cc: netdev, David S. Miller, Harald Welte, Johannes Berg, Pablo Neira,
LKML, kernel-janitors
In-Reply-To: <91f31650-3052-507c-8006-96874107f981@users.sourceforge.net>
On Thu, Jan 26, 2017 at 1:18 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> - pctx = kmalloc(sizeof(struct pdp_ctx), GFP_KERNEL);
> + pctx = kmalloc(sizeof(*pctx), GFP_KERNEL);
The rule about "sizeof(*p)" style of allocation is bogus and should be
abolished.
Rationale says that
> The alternative form where struct name is spelled out hurts readability
In terms of line length, yes, "sizeof(*p)" wins most of the time.
However, the former variant clearly show the type of data, so you don't need
to look it up and makes it readily available to follow with tags or equivalent.
> and introduces an opportunity for a bug when the pointer variable type
> is changed but the corresponding sizeof that is passed to a memory
> allocator is not.
The correct way to prevent this kind of mistake is to not return "void
*" pointer
(at least most of the uses use typed allocation)
#define lmalloc(T, gfp) (T*)_kmalloc(sizeof(T), (gfp))
Bacause of a cast changing type of pointer will be noticed,
^ permalink raw reply
* Re: [PATCH v4 net] ravb: unmap descriptors when freeing rings
From: Simon Horman @ 2017-01-26 13:24 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: David Miller, Magnus Damm, netdev, linux-renesas-soc,
Kazuya Mizuguchi
In-Reply-To: <d6a3fdea-1934-3983-6897-0b203dea51df@cogentembedded.com>
On Thu, Jan 26, 2017 at 03:58:45PM +0300, Sergei Shtylyov wrote:
> On 01/26/2017 02:04 PM, Simon Horman wrote:
>
> >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >
> >"swiotlb buffer is full" errors occur after repeated initialisation of a
> >device - f.e. suspend/resume or ip link set up/down. This is because memory
> >mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit()
> >is not released. Resolve this problem by unmapping descriptors when
> >freeing rings.
> >
> >Fixes: a780893c89f6 ("ravb: unmap descriptors when freeing rings")
>
> No such SHA1 hash known and the summary is from this patch.
Sorry, it took the HEAD commit rather than a hash I should have provided
as an argument to git fixes:
[alias]
fixes = log -1 --pretty=fixes
[core]
abbrev = 12
[pretty]
fixes = Fixes: %h (\"%s\")
What I wanted to say was:
Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> >[simon: reworked]
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >--
> >
> >On inspection it appears that a similar problem is also present in
> >the SH-Ethernet driver.
> >
> >v4 [Simon Horman]
> >* As suggested by Sergei Shtylyov
> > - Use bool as type for new parameter to ravb_tx_free()
> > - Clean up comment style
> > - Only increment tx_bytes for transmitted skbs
> >
> >v3 [Simon Horman]
> >* As suggested by Sergei Shtylyov
> > - consistently use le32_to_cpu(desc->dptr)
> > - Do not clear desc->ds_cc as it is not used
> >* Paramatise ravb_tx_free() to allow it to free non-transmitted buffers
> >
> >v2 [Simon Horman]
> >* As suggested by Sergei Shtylyov
> > - Use dma_mapping_error() and rx_desc->ds_cc when unmapping RX descriptors;
> > this is consistent with the way that they are mapped
> > - Use ravb_tx_free() to clear TX descriptors
> >* Reduce scope of new local variable
> >
> >v1 [Kazuya Mizuguchi]
> >---
> > drivers/net/ethernet/renesas/ravb_main.c | 111 ++++++++++++++++++-------------
> > 1 file changed, 63 insertions(+), 48 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >index 89ac1e3f6175..5d66ac721335 100644
> >--- a/drivers/net/ethernet/renesas/ravb_main.c
> >+++ b/drivers/net/ethernet/renesas/ravb_main.c
> >@@ -179,6 +179,48 @@ static struct mdiobb_ops bb_ops = {
> > .get_mdio_data = ravb_get_mdio_data,
> > };
> >
> >+/* Free TX skb function for AVB-IP */
> >+static int ravb_tx_free(struct net_device *ndev, int q, bool free_txed_only)
> >+{
> >+ struct ravb_private *priv = netdev_priv(ndev);
> >+ struct net_device_stats *stats = &priv->stats[q];
> >+ struct ravb_tx_desc *desc;
> >+ int free_num = 0;
> >+ int entry;
> >+ u32 size;
> >+
> >+ for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) {
> >+ bool txed;
> >+
> >+ entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] *
> >+ NUM_TX_DESC);
> >+ desc = &priv->tx_ring[q][entry];
> >+ txed = desc->die_dt == DT_FEMPTY;
> >+ if (free_txed_only && !txed)
> >+ break;
> >+ /* Descriptor type must be checked before all other reads */
> >+ dma_rmb();
> >+ size = le16_to_cpu(desc->ds_tagl) & TX_DS;
> >+ /* Free the original skb. */
> >+ if (priv->tx_skb[q][entry / NUM_TX_DESC]) {
> >+ dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> >+ size, DMA_TO_DEVICE);
> >+ /* Last packet descriptor? */
> >+ if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) {
> >+ entry /= NUM_TX_DESC;
> >+ dev_kfree_skb_any(priv->tx_skb[q][entry]);
> >+ priv->tx_skb[q][entry] = NULL;
> >+ stats->tx_packets++;
>
> We're still incrementing the packet # even when the packet hasn't been sent...
Thanks, sorry for missing that.
> >+ }
> >+ free_num++;
> >+ }
> >+ if (!txed)
>
> Your logic is reversed! :-(
Oops.
> >+ stats->tx_bytes += size;
> >+ desc->die_dt = DT_EEMPTY;
> >+ }
> >+ return free_num;
> >+}
> >+
> > /* Free skb's and DMA buffers for Ethernet AVB */
> > static void ravb_ring_free(struct net_device *ndev, int q)
> > {
> >@@ -194,19 +236,21 @@ static void ravb_ring_free(struct net_device *ndev, int q)
> [...]
> >
> > if (priv->rx_ring[q]) {
> >+ for (i = 0; i < priv->num_rx_ring[q]; i++) {
> >+ struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
> >+
> >+ if (!dma_mapping_error(ndev->dev.parent,
> >+ le32_to_cpu(desc->dptr)))
>
> I'd check for zero buffer length (that marks unmapped RX buffers) but
> this should be good too... it's just that the length check is shorter. :-)
I'm pretty sure you suggested the logic above at some point.
Lets keep it :)
> >+ dma_unmap_single(ndev->dev.parent,
> >+ le32_to_cpu(desc->dptr),
> >+ PKT_BUF_SZ,
> >+ DMA_FROM_DEVICE);
> >+ }
> > ring_size = sizeof(struct ravb_ex_rx_desc) *
> > (priv->num_rx_ring[q] + 1);
> > dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q],
> [...]
>
> MBR, Sergei
>
^ permalink raw reply
* IPv6-UDP 0x0000 checksum
From: Johannes Berg @ 2017-01-26 13:27 UTC (permalink / raw)
To: netdev; +Cc: linux-wireless
Hi,
It looks like right now we may have a hardware bug and accept 0x0000 as
valid, when the outcome of the calculation is 0xffff.
What do you think we should do about this?
We could ignore the issue entirely, since 0 wasn't ever supposed to be
sent anyway - but then we don't drop frames that we should drop. I
didn't manage to find the code in the IPv6/UDP stack that even does
that, but I assume it's there somewhere.
Alternatively, we could parse the packet to find the checksum inside,
and if it's 0 then don't report CHECKSUM_UNNECESSARY, but that seems
rather expensive/difficult due to the IPv6 variable header and all
that. If we wanted to go this route, are there any helper functions for
this?
Unfortunately, in the current devices, we neither have a complete
indication that the packet was even UDP-IPv6, nor do we have the raw
csum or anything like that. I think they're adding that to the next
hardware spin, but we probably need to address this issue now.
johannes
^ permalink raw reply
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