Re: INFO: rcu detected stall in corrupted (4)
Dmitry Vyukov writes: > On Fri, Sep 4, 2020 at 8:49 PM syzbot > wrote: >> >> Hello, >> >> syzbot found the following issue on: >> >> HEAD commit:0f091e43 netlabel: remove unused param from audit_log_form.. >> git tree: net-next >> console output: https://syzkaller.appspot.com/x/log.txt?x=14551a7190 >> kernel config: https://syzkaller.appspot.com/x/.config?x=61025c6fd3261bb1 >> dashboard link: https://syzkaller.appspot.com/bug?extid=aa7d098bd6fa788fae8e >> compiler: gcc (GCC) 10.1.0-syz 20200507 >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14eeda2590 >> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=161472f590 >> >> The issue was bisected to: >> >> commit 5a781ccbd19e4664babcbe4b4ead7aa2b9283d22 >> Author: Vinicius Costa Gomes >> Date: Sat Sep 29 00:59:43 2018 + >> >> tc: Add support for configuring the taprio scheduler > > This still happens. The bisection and repro look correct, the repro > also sets up taprio scheduler; > https://syzkaller.appspot.com/bug?id=7349616606afa3c986c377792f7ccbf9daae1142 > > Vinicius, could you please take a look? Thanks Ugh. Sure. Taking a look. Cheers, -- Vinicius
Similar functions in net/core/dev.c
Hi, All these functions in net/core/dev.c are very similar. __netdev_walk_all_upper_dev netdev_walk_all_upper_dev_rcu netdev_walk_all_lower_dev __netdev_walk_all_lower_dev netdev_walk_all_lower_dev_rcu Can they be merged in one function? Are they a hot path? Regards, Vinicius
Re: [PATCH net 1/1] net: stmmac: set TxQ mode back to DCB after disabling CBS
Song Yoong Siang writes:
> From: Mohammad Athari Bin Ismail
>
> When disable CBS, mode_to_use parameter is not updated even the operation
> mode of Tx Queue is changed to Data Centre Bridging (DCB). Therefore,
> when tc_setup_cbs() function is called to re-enable CBS, the operation
> mode of Tx Queue remains at DCB, which causing CBS fails to work.
>
> This patch updates the value of mode_to_use parameter to MTL_QUEUE_DCB
> after operation mode of Tx Queue is changed to DCB in stmmac_dma_qmode()
> callback function.
>
> Fixes: 1f705bc61aee ("net: stmmac: Add support for CBS QDISC")
> Suggested-by: Gomes, Vinicius
Just a nitpick/formality, I would prefer if you used:
Suggested-by: Vinicius Costa Gomes
> Signed-off-by: Mohammad Athari Bin Ismail
> Signed-off-by: Song, Yoong Siang
Patch looks good.
Acked-by: Vinicius Costa Gomes
Cheers,
--
Vinicius
Re: [PATCH v3] ext4: Enable code path when DX_DEBUG is set
On Wed, Feb 3, 2021 at 2:39 AM Theodore Ts'o wrote:
>
> On Tue, Feb 02, 2021 at 04:28:37PM +, Vinicius Tinti wrote:
> > Clang with -Wunreachable-code-aggressive is being used to try to find
> > unreachable code that could cause potential bugs. There is no plan to
> > enable it by default.
> >
> > The following code was detected as unreachable:
> >
> > fs/ext4/namei.c:831:17: warning: code will never be executed
> > [-Wunreachable-code]
> > unsigned n = count - 1;
> > ^
> > fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> > explicitly dead
> > if (0) { // linear search cross check
> > ^
> > /* DISABLES CODE */ ( )
> >
> > This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
> > copy of files from ext3") and fs/ext3 had it present at the beginning of
> > git history. It has not been changed since.
> >
> > This patch moves the code to a new function `htree_rep_invariant_check`
> > which only performs the check when DX_DEBUG is set. This allows the
> > function to be used in other parts of the code.
> >
> > Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.
> >
> > Signed-off-by: Vinicius Tinti
>
> Thanks, applied, although I rewrote the commit description:
>
> ext4: factor out htree rep invariant check
>
> This patch moves some debugging code which is used to validate the
> hash tree node when doing a binary search of an htree node into a
> separate function, which is disabled by default (since it is only used
> by developers when they are modifying the htree code paths).
>
> In addition to cleaning up the code to make it more maintainable, it
> silences a Clang compiler warning when -Wunreachable-code-aggressive
> is enabled. (There is no plan to enable this warning by default, since
> there it has far too many false positives; nevertheless, this commit
> reduces one of the many false positives by one.)
>
> The original description buried the lede, in terms of the primary
> reason why I think the change was worthwhile (although I know you have
> different priorities than mine :-).
>
> Thanks for working to find a way to improve the code in a way that
> makes both of us happy!
Thanks for the feedback.
And thanks for all the ones who reviewed.
> - Ted
[PATCH v3] ext4: Enable code path when DX_DEBUG is set
Clang with -Wunreachable-code-aggressive is being used to try to find
unreachable code that could cause potential bugs. There is no plan to
enable it by default.
The following code was detected as unreachable:
fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
unsigned n = count - 1;
^
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
if (0) { // linear search cross check
^
/* DISABLES CODE */ ( )
This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history. It has not been changed since.
This patch moves the code to a new function `htree_rep_invariant_check`
which only performs the check when DX_DEBUG is set. This allows the
function to be used in other parts of the code.
Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.
Signed-off-by: Vinicius Tinti
---
fs/ext4/namei.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..a6e28b4b5a95 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -731,6 +731,29 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo,
struct inode *dir,
(space/bcount)*100/blocksize);
return (struct stats) { names, space, bcount};
}
+
+/*
+ * Linear search cross check
+ */
+static inline void htree_rep_invariant_check(struct dx_entry *at,
+struct dx_entry *target,
+u32 hash, unsigned int n)
+{
+ while (n--) {
+ dxtrace(printk(KERN_CONT ","));
+ if (dx_get_hash(++at) > hash) {
+ at--;
+ break;
+ }
+ }
+ ASSERT(at == target - 1);
+}
+#else /* DX_DEBUG */
+static inline void htree_rep_invariant_check(struct dx_entry *at,
+struct dx_entry *target,
+u32 hash, unsigned int n)
+{
+}
#endif /* DX_DEBUG */
/*
@@ -827,20 +850,7 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
p = m + 1;
}
- if (0) { // linear search cross check
- unsigned n = count - 1;
- at = entries;
- while (n--)
- {
- dxtrace(printk(KERN_CONT ","));
- if (dx_get_hash(++at) > hash)
- {
- at--;
- break;
- }
- }
- ASSERT(at == p - 1);
- }
+ htree_rep_invariant_check(entries, p, hash, count - 1);
at = p - 1;
dxtrace(printk(KERN_CONT " %x->%u\n",
--
2.25.1
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers wrote:
>
> On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o wrote:
> >
> > On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote:
> > > I agree; Vinicius, my recommendation for -Wunreachable-* with Clang
> > > was to see whether dead code identified by this more aggressive
> > > diagnostic (than -Wunused-function) was to ask maintainers whether
> > > code identified by it was intentionally dead and if they would
> > > consider removing it. If they say "no," that's fine, and doesn't need
> > > to be pushed. It's not clear to maintainers that:
> > > 1. this warning is not on by default
> > > 2. we're not looking to pursue turning this on by default
Ok. I will make it clear in next commit messages.
> > >
> > > If maintainers want to keep the dead code, that's fine, let them and
> > > move on to the next instance to see if that's interesting (or not).
> >
> > It should be noted that in Documenting/process/coding-style.rst, there
> > is an expicit recommendation to code in a way that will result in dead
> > code warnings:
> >
> >Within code, where possible, use the IS_ENABLED macro to convert a
> > Kconfig
> >symbol into a C boolean expression, and use it in a normal C conditional:
> >
> >.. code-block:: c
> >
> > if (IS_ENABLED(CONFIG_SOMETHING)) {
> > ...
> > }
> >
> >The compiler will constant-fold the conditional away, and include or
> > exclude
> >the block of code just as with an #ifdef, so this will not add any
> > runtime
> >overhead. However, this approach still allows the C compiler to see the
> > code
> >inside the block, and check it for correctness (syntax, types, symbol
> >references, etc). Thus, you still have to use an #ifdef if the code
> > inside the
> >block references symbols that will not exist if the condition is not met.
> >
> > So our process documentation *explicitly* recommends against using
> > #ifdef CONFIG_XXX ... #endif, and instead use something that will
> > -Wunreachable-code-aggressive to cause the compiler to complain.
>
> I agree.
I agree too.
> >
> > Hence, this is not a warning that we will *ever* be able to enable
> > unconditionally ---
>
> I agree.
>
> > so why work hard to remove such warnings from the
> > code? If the goal is to see if we can detect real bugs using this
>
> Because not every instance of -Wunreachable-code-aggressive may be that
> pattern.
The goal is to try to detect real bugs. In this instance specifically I
suggested to remove the "if (0) {...}" because it sounded like an
unused code.
If it is useful it is fine to keep.
For now I am only looking for dead code that cannot be enabled
by a configuration file or architecture. In fact, there are several
warnings that I am ignoring because they are a dead code in my
build but may not be in another.
> > technique, well and good. If the data shows that this warning
> > actually is useful in finding bugs, then manybe we can figure out a
> > way that we can explicitly hint to the compiler that in *this* case,
> > the maintainer actually knew what they were doing.
> >
> > But if an examination of the warnings shows that
> > -Wunreachable-code-aggressive isn't actually finding any real bugs,
> > then perhaps it's not worth it.
>
> I agree. Hence the examination of instances found by Vinicius.
I liked the idea to create htree_rep_invariant_check. I will be doing
that.
Thanks for the help and suggestions.
> --
> Thanks,
> ~Nick Desaulniers
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o wrote: > > On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote: > > On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > > > > > DX_DEBUG is completely dead code, so either kill it off or make it an > > > actual CONFIG_* symbol through Kconfig if it seems useful. > > > > About the unreachable code in "if (0)" I think it could be removed. > > It seems to be doing an extra check. > > > > About the DX_DEBUG I think I can do another patch adding it to Kconfig > > as you and Nathan suggested. > > Yes, it's doing another check which is useful in terms of early > detection of bugs when a developer has the code open for > modifications. It slows down performance under normal circumstances, > and assuming the code is bug-free(tm), it's entirely unnecessary --- > which is why it's under an "if (0)". My goal is to avoid having a dead code. Three options come to mind. The first would be to add another #ifdef SOMETHING (suggest a name). But this doesn't remove the code and someone could enable it by accident. The second would be to add the code in a block comment. Then document that it is for checking an invariant. This will make it harder to cause problems. The third would be to remove the code and explain the invariant in a block comment. Maybe add a pseudo code too in the comment. What do you think? > However, if there *is* a bug, having an early detection that the > representation invariant of the data structure has been violated can > be useful in root causing a bug. This would probably be clearer if > the code was pulled out into a separate function with comments > explaining that this is a rep invariant check. Good idea. I will do that too. > The main thing about DX_DEBUG right now is that it is **super** > verbose. Unwary users who enable it will be sorry. If we want to > make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we > should convert all of the dx_trace calls to use pr_debug so they are > enabled only if dynamic debug enables those pr_debug() statements. > And this should absolutely be a separate patch. Agreed. For now I only want to focus on the "if (0)". Regards, Vinicius > Cheers, > > - Ted
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig wrote: > > DX_DEBUG is completely dead code, so either kill it off or make it an > actual CONFIG_* symbol through Kconfig if it seems useful. About the unreachable code in "if (0)" I think it could be removed. It seems to be doing an extra check. About the DX_DEBUG I think I can do another patch adding it to Kconfig as you and Nathan suggested. What do you think?
[PATCH v2] ext4: Enable code path when DX_DEBUG is set
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history. It has not been changed since.
Clang warns:
fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
unsigned n = count - 1;
^
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
if (0) { // linear search cross check
^
/* DISABLES CODE */ ( )
Signed-off-by: Vinicius Tinti
---
fs/ext4/namei.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..46ae6a4e4be5 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -827,20 +827,21 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
p = m + 1;
}
- if (0) { // linear search cross check
- unsigned n = count - 1;
- at = entries;
- while (n--)
+#ifdef DX_DEBUG
+ // linear search cross check
+ unsigned n = count - 1;
+ at = entries;
+ while (n--)
+ {
+ dxtrace(printk(KERN_CONT ","));
+ if (dx_get_hash(++at) > hash)
{
- dxtrace(printk(KERN_CONT ","));
- if (dx_get_hash(++at) > hash)
- {
- at--;
- break;
- }
+ at--;
+ break;
}
- ASSERT(at == p - 1);
}
+ ASSERT(at == p - 1);
+#endif
at = p - 1;
dxtrace(printk(KERN_CONT " %x->%u\n",
--
2.25.1
Re: [PATCH] drm/i915: Remove unreachable code
On Sat, Jan 30, 2021 at 9:45 AM Chris Wilson wrote:
>
> Quoting Vinicius Tinti (2021-01-30 12:34:11)
> > On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote:
> > > Quoting Vinicius Tinti (2021-01-29 18:15:19)
> > > > By enabling -Wunreachable-code-aggressive on Clang the following code
> > > > paths are unreachable.
> > >
> > > That code exists as commentary and, especially for sdvo, library
> > > functions that we may need in future.
> >
> > I would argue that this code could be removed since it is in git history.
> > It can be restored when needed.
> >
> > This will make the code cleaner.
>
> It doesn't change the control flow, so no complexity argument. It
> removes documentation from the code, so I have the opposite opinion.
The last change in sdvo related to this function is from commit
ce22c320b8ca ("drm/i915/sdvo: convert to encoder disable/enable"), which
dates from 2012.
It has not been used or changed for a long time. I think it could be
converted to a block comment. This will preserve the documentation
purpose. What do you think?
All this will avoid having "if (0)".
> > > The ivb-gt1 case => as we now set the gt level for ivb, should we not
> > > enable the optimisation for ivb unaffected by the w/a? Just no one has
> > > taken the time to see if it causes a regression.
> >
> > I don't know. I just found out that the code is unreachable.
> >
> > > For error state, the question remains whether we should revert to
> > > uncompressed data if the compressed stream is larger than the original.
> >
> > I don't know too.
> >
> > In this last two cases the code could be commented and the decisions
> > and problems explained in the comment section.
>
> They already are, that is the point.
I meant making them a block comment. For example:
/*
* Enabling HiZ Raw Stall Optimization, at this point, causes corruption.
*
* Calling wa_masked_dis with the arguments wal, CACHE_MODE_0_GEN7,
* HIZ_RAW_STALL_OPT_DISABLE will cause an HiZ corruption on ivb:g1.
*/
/*
* Should fallback to uncompressed if we increase size
* (zstream->total_out > zstream->total_in) by returning -E2BIG?
*/
> -Chris
Re: [PATCH] drm/i915: Remove unreachable code
On Fri, Jan 29, 2021 at 08:55:54PM +, Chris Wilson wrote: > Quoting Vinicius Tinti (2021-01-29 18:15:19) > > By enabling -Wunreachable-code-aggressive on Clang the following code > > paths are unreachable. > > That code exists as commentary and, especially for sdvo, library > functions that we may need in future. I would argue that this code could be removed since it is in git history. It can be restored when needed. This will make the code cleaner. > The ivb-gt1 case => as we now set the gt level for ivb, should we not > enable the optimisation for ivb unaffected by the w/a? Just no one has > taken the time to see if it causes a regression. I don't know. I just found out that the code is unreachable. > For error state, the question remains whether we should revert to > uncompressed data if the compressed stream is larger than the original. I don't know too. In this last two cases the code could be commented and the decisions and problems explained in the comment section. > -Chris
[PATCH] ext4: Remove unreachable code
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
Commit dd73b5d5cb67 ("ext4: convert dx_probe() to use the ERR_PTR
convention")
Commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
Clang warns:
fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
unsigned n = count - 1;
^
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
if (0) { // linear search cross check
^
/* DISABLES CODE */ ( )
Signed-off-by: Vinicius Tinti
---
fs/ext4/namei.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..1f64dbd7237b 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -827,21 +827,6 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
p = m + 1;
}
- if (0) { // linear search cross check
- unsigned n = count - 1;
- at = entries;
- while (n--)
- {
- dxtrace(printk(KERN_CONT ","));
- if (dx_get_hash(++at) > hash)
- {
- at--;
- break;
- }
- }
- ASSERT(at == p - 1);
- }
-
at = p - 1;
dxtrace(printk(KERN_CONT " %x->%u\n",
at == entries ? 0 : dx_get_hash(at),
--
2.25.1
[PATCH] drm/i915: Remove unreachable code
By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.
Commit ce22c320b8ca ("drm/i915/sdvo: convert to encoder disable/enable")
Commit 19f1f627b333 ("drm/i915/gt: Move ivb GT workarounds from
init_clock_gating to workarounds")
Commit 0a97015d45ee ("drm/i915: Compress GPU objects in error state")
By removing the unreachable code at
drivers/gpu/drm/i915/display/intel_sdvo.c the function
intel_sdvo_set_encoder_power_state becomes unused.
Commit ea5b213ad4b1 ("drm/i915: Subclass intel_encoder.")
Clang warns unreachable:
drivers/gpu/drm/i915/display/intel_sdvo.c:1768:3: warning: code will never
be executed [-Wunreachable-code]
intel_sdvo_set_encoder_power_state(intel_sdvo,
^~
drivers/gpu/drm/i915/display/intel_sdvo.c:1767:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0)
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/display/intel_sdvo.c:1852:3: warning: code will never
be executed [-Wunreachable-code]
intel_sdvo_set_encoder_power_state(intel_sdvo,
^~
drivers/gpu/drm/i915/display/intel_sdvo.c:1851:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0)
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/gt/intel_workarounds.c:884:3: warning: code will never
be executed [-Wunreachable-code]
wa_masked_dis(wal, CACHE_MODE_0_GEN7,
HIZ_RAW_STALL_OPT_DISABLE);
^
drivers/gpu/drm/i915/gt/intel_workarounds.c:882:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0) { /* causes HiZ corruption on ivb:gt1 */
^
/* DISABLES CODE */ ( )
drivers/gpu/drm/i915/i915_gpu_error.c:319:11: warning: code will never be
executed [-Wunreachable-code]
if (0 && zstream->total_out > zstream->total_in)
^~~
drivers/gpu/drm/i915/i915_gpu_error.c:319:6: note: silence by adding
parentheses to mark code as explicitly dead
if (0 && zstream->total_out > zstream->total_in)
^
/* DISABLES CODE */ ( )
Clang warns unused after removing unreachable:
drivers/gpu/drm/i915/display/intel_sdvo.c:696:13: warning: unused function
'intel_sdvo_set_encoder_power_state' [-Wunused-function]
static bool intel_sdvo_set_encoder_power_state(struct intel_sdvo *intel_sdvo,
^
Signed-off-by: Vinicius Tinti
---
drivers/gpu/drm/i915/display/intel_sdvo.c | 30 -
drivers/gpu/drm/i915/gt/intel_workarounds.c | 5
drivers/gpu/drm/i915/i915_gpu_error.c | 4 ---
3 files changed, 39 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 4eaa4aa86ecd..45d03b09f8f0 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -693,30 +693,6 @@ static bool intel_sdvo_get_active_outputs(struct
intel_sdvo *intel_sdvo,
outputs, sizeof(*outputs));
}
-static bool intel_sdvo_set_encoder_power_state(struct intel_sdvo *intel_sdvo,
- int mode)
-{
- u8 state = SDVO_ENCODER_STATE_ON;
-
- switch (mode) {
- case DRM_MODE_DPMS_ON:
- state = SDVO_ENCODER_STATE_ON;
- break;
- case DRM_MODE_DPMS_STANDBY:
- state = SDVO_ENCODER_STATE_STANDBY;
- break;
- case DRM_MODE_DPMS_SUSPEND:
- state = SDVO_ENCODER_STATE_SUSPEND;
- break;
- case DRM_MODE_DPMS_OFF:
- state = SDVO_ENCODER_STATE_OFF;
- break;
- }
-
- return intel_sdvo_set_value(intel_sdvo,
- SDVO_CMD_SET_ENCODER_POWER_STATE, ,
sizeof(state));
-}
-
static bool intel_sdvo_get_input_pixel_clock_range(struct intel_sdvo
*intel_sdvo,
int *clock_min,
int *clock_max)
@@ -1764,9 +1740,6 @@ static void intel_disable_sdvo(struct intel_atomic_state
*state,
intel_sdvo_disable_audio(intel_sdvo);
intel_sdvo_set_active_outputs(intel_sdvo, 0);
- if (0)
- intel_sdvo_set_encoder_power_state(intel_sdvo,
- DRM_MODE_DPMS_OFF);
temp = intel_de_read(dev_priv, intel_sdvo->sdvo_reg);
@@ -1848,9 +1821,6 @@ static void intel_enable_sdvo(struct intel_atomic_state
*state,
"sync\n", SDVO_NAME(intel_sdvo));
}
- if (0)
- intel_sdvo_set_encoder_power_state(intel_sdvo,
- DRM_MODE_DPMS_ON);
intel_sdvo_set_active
Re: [PATCH 1/3] Add TX sending hardware timestamp.
Willem de Bruijn writes: >> > If I understand correctly, you are trying to achieve a single delivery >> > time. >> > The need for two separate timestamps passed along is only because the >> > kernel is unable to do the time base conversion. >> >> Yes, a correct point. >> >> > >> > Else, ETF could program the qdisc watchdog in system time and later, >> > on dequeue, convert skb->tstamp to the h/w time base before >> > passing it to the device. >> >> Or the skb->tstamp is HW time-stamp and the ETF convert it to system clock >> based. >> >> > >> > It's still not entirely clear to me why the packet has to be held by >> > ETF initially first, if it is held until delivery time by hardware >> > later. But more on that below. >> >> Let plot a simple scenario. >> App A send a packet with time-stamp 100. >> After arrive a second packet from App B with time-stamp 90. >> Without ETF, the second packet will have to wait till the interface hardware >> send the first packet on 100. >> Making the second packet late by 10 + first packet send time. >> Obviously other "normal" packets are send to the non-ETF queue, though they >> do not block ETF packets >> The ETF delta is a barrier that the application have to send the packet >> before to ensure the packet do not tossed. > > Got it. The assumption here is that devices are FIFO. That is not > necessarily the case, but I do not know whether it is in practice, > e.g., on the i210. On the i210 and i225, that's indeed the case, i.e. only the launch time of the packet at the front of the queue is considered. [...] >> >>>>> It only requires that pacing qdiscs, both sch_etf and sch_fq, >> >>>>> optionally skip queuing in their .enqueue callback and instead allow >> >>>>> the skb to pass to the device driver as is, with skb->tstamp set. Only >> >>>>> to devices that advertise support for h/w pacing offload. >> >>>>> >> >>>> I did not use "Fair Queue traffic policing". >> >>>> As for ETF, it is all about ordering packets from different >> >>>> applications. >> >>>> How can we achive it with skiping queuing? >> >>>> Could you elaborate on this point? >> >>> >> >>> The qdisc can only defer pacing to hardware if hardware can ensure the >> >>> same invariants on ordering, of course. >> >> >> >> Yes, this is why we suggest ETF order packets using the hardware >> >> time-stamp. >> >> And pass the packet based on system time. >> >> So ETF query the system clock only and not the PHC. >> > >> > On which note: with this patch set all applications have to agree to >> > use h/w time base in etf_enqueue_timesortedlist. In practice that >> > makes this h/w mode a qdisc used by a single process? >> >> A single process theoretically does not need ETF, just set the skb-> tstamp >> and use a pass through queue. >> However the only way now to set TC_SETUP_QDISC_ETF in the driver is using >> ETF. > > Yes, and I'd like to eventually get rid of this constraint. > I'm interested in these kind of ideas :-) What would be your end goal? Something like: - Any application is able to set SO_TXTIME; - We would have a best effort support for scheduling packets based on their transmission time enabled by default; - If the hardware supports, there would be a "offload" flag that could be enabled; More or less this? Cheers. -- Vinicius
Re: Why ping latency is smaller with shorter send interval?
Hi, Eric Dumazet writes: > > Many factors in play here. > > 1) if you keep cpus busy enough, they tend to keep in their caches > the data needed to serve your requests. In your case, time taken to > process an ICMP packet can be very different depending on how hot > cpu caches are. > > 2) Idle cpus can be put in a power conserving state. > It takes time to exit from these states, as you noticed. > These delays can typically be around 50 usec, or more. Still on the power management theme, in my experience, in addition to the CPU power states, the PCIe and NIC power management settings also effect latency on the order of 10-100s usecs when the system is allowed to go idle, some things that have helped: - setting CONFIG_PCIEASPM to performance; - disabling EEE (energy efficient ethernet) in your NIC; > > Search for cpu C-states , and powertop program. > Cheers, -- Vinicius
Re: [PATCH 0/7] TC-ETF support PTP clocks series
Hi Erez, Erez Geva writes: > Add support for using PTP clock with > Traffic control Earliest TxTime First (ETF) Qdisc. > > Why do we need ETF to use PTP clock? > Current ETF requires to synchronization the system clock > to the PTP Hardware clock (PHC) we want to send through. > But there are cases that we can not synchronize the system clock with > the desire NIC PHC. > 1. We use several NICs with several PTP domains that our device > is not allowed to be PTP master. >And we are not allowed to synchronize these PTP domains. > 2. We are using another clock source which we need for our system. >Yet our device is not allowed to be PTP master. > Regardless of the exact topology, as the Linux tradition is to allow > the user the freedom to choose, we propose a patch that will allow > the user to configure the TC-ETF with a PTP clock as well as > using the system clock. > * NOTE: we do encourage the users to synchronize the system clock with > a PTP clock. > As the ETF watchdog uses the system clock. > Synchronizing the system clock with a PTP clock will probably > reduce the frequency different of the PHC and the system clock. > As sequence, the user might be able to reduce the ETF delta time > and the packets latency cross the network. > > Follow the decision to derive a dynamic clock ID from the file description > of an opened PTP clock device file. > We propose a simple way to use the dynamic clock ID with the ETF Qdisc. > We will submit a patch to the "tc" tool from the iproute2 project > once this patch is accepted. > In addition to what Thomas said, I would like to add some thoughts (mostly re-wording some of Thomas' comments :-)). I think that there's an underlying problem/limitation that is the cause of the issue (or at least a step in the right direction) you are trying to solve: the issue is that PTP clocks can't be used as hrtimers. I didn't spend a lot of time thinking about how to solve this (the only thing that comes to mind is having a timecounter, or similar, "software view" over the PHC clock). Anyway, my feeling is that until this is solved, we would only be working around the problem, and creating even more hard to handle corner cases. Cheers, -- Vinicius
Re: INFO: rcu detected stall in cleanup_net (4)
Hi Jakub, Jakub Kicinski writes: > On Tue, 08 Sep 2020 22:29:21 -0700 syzbot wrote: >> Hello, >> >> syzbot found the following issue on: >> >> HEAD commit:59126901 Merge tag 'perf-tools-fixes-for-v5.9-2020-09-03' .. >> git tree: upstream >> console output: https://syzkaller.appspot.com/x/log.txt?x=12edb93590 >> kernel config: https://syzkaller.appspot.com/x/.config?x=3c5f6ce8d5b68299 >> dashboard link: https://syzkaller.appspot.com/bug?extid=8267241609ae8c23b248 >> compiler: gcc (GCC) 10.1.0-syz 20200507 >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=157c7aa590 >> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13c92ef990 >> >> The issue was bisected to: >> >> commit 5a781ccbd19e4664babcbe4b4ead7aa2b9283d22 >> Author: Vinicius Costa Gomes >> Date: Sat Sep 29 00:59:43 2018 + >> >> tc: Add support for configuring the taprio scheduler > > > Vinicius, could you please take a look at all the syzbot reports which > point to your commit? I know syzbot bisection is not super reliable, > but at least 3 reports point to your commit now, so something's > probably going on. I did take a look, and it seems that it all boils down to having too small (unreasonable?) intervals in the schedule, which causes the hrtimer handler to starve the other kernel threads. I have a quick fix to restrict the interval values to more sensible values (at least equal to the time it takes to transmit the mininum ethernet frame size), I am testing it and I will propose it soon. But a proper solution will require more time. Cheers, -- Vinicius
Re: 回复: INFO: rcu detected stall in tc_modify_qdisc
Dmitry Vyukov writes: >> > >> > Also are we talking about CAP_NET_ADMIN in a user ns as well >> > (effectively nobody)? >> >> Just checked, we are talking about CAP_NET_ADMIN in user namespace as >> well. > > OK, so this is not root/admin, this is just any user. Yeah, will fix this. Thanks, -- Vinicius
Re: 回复: INFO: rcu detected stall in tc_modify_qdisc
Hi Eric,
Eric Dumazet writes:
>> I admit that I am on the fence on that argument: do not let even root
>> crash the system (the point that my code is crashing the system gives
>> weight to this side) vs. root has great powers, they need to know what
>> they are doing.
>>
>> The argument that I used to convince myself was: root can easily create
>> a bunch of processes and give them the highest priority and do
>> effectively the same thing as this issue, so I went with a the "they
>> need to know what they are doing side".
>>
>> A bit more on the specifics here:
>>
>> - Using a small interval size, is only a limitation of the taprio
>> software mode, when using hardware offloads (which I think most users
>> do), any interval size (supported by the hardware) can be used;
>>
>> - Choosing a good lower limit for this seems kind of hard: something
>> below 1us would never work well, I think, but things 1us < x < 100us
>> will depend on the hardware/kernel config/system load, and this is the
>> range includes "useful" values for many systems.
>>
>> Perhaps a middle ground would be to impose a limit based on the link
>> speed, the interval can never be smaller than the time it takes to send
>> the minimum ethernet frame (for 1G links this would be ~480ns, should be
>> enough to catch most programming mistakes). I am going to add this and
>> see how it looks like.
>>
>> Sorry for the brain dump :-)
>
>
> I do not know taprio details, but do you really need a periodic timer
> ?
As we can control the transmission time of packets, you are right, I
don't.
Just a bit more detail about the current implementation taprio,
basically it has a sequence of { Traffic Classes that are open; Interval
} that repeats cyclicly, it uses an hrtimer to advance the pointer for
the current element, so during dequeue I can check if a traffic class is
"open" or "closed".
But again, if I calculate the 'skb->tstamp' of each packet during
enqueue, I don't need the hrtimer. What we have in the txtime-assisted
mode is half way there.
I think this is what you had in mind.
Cheers,
--
Vinicius
Re: 回复: INFO: rcu detected stall in tc_modify_qdisc
Hi, Dmitry Vyukov writes: > On Wed, Jul 29, 2020 at 9:13 PM Vinicius Costa Gomes > wrote: >> >> Hi, >> >> "Zhang, Qiang" writes: >> >> > >> > 发件人: linux-kernel-ow...@vger.kernel.org >> > 代表 syzbot >> > >> > 发送时间: 2020年7月29日 13:53 >> > 收件人: da...@davemloft.net; fweis...@gmail.com; j...@mojatatu.com; >> > j...@resnulli.us; linux-kernel@vger.kernel.org; mi...@kernel.org; >> > net...@vger.kernel.org; syzkaller-b...@googlegroups.com; >> > t...@linutronix.de; vinicius.go...@intel.com; xiyou.wangc...@gmail.com >> > 主题: INFO: rcu detected stall in tc_modify_qdisc >> > >> > Hello, >> > >> > syzbot found the following issue on: >> > >> > HEAD commit:181964e6 fix a braino in cmsghdr_from_user_compat_to_kern() >> > git tree: net >> > console output: https://syzkaller.appspot.com/x/log.txt?x=12925e3890 >> > kernel config: https://syzkaller.appspot.com/x/.config?x=f87a5e4232fdb267 >> > dashboard link: >> > https://syzkaller.appspot.com/bug?extid=9f78d5c664a8c33f4cce >> > compiler: gcc (GCC) 10.1.0-syz 20200507 >> > syz repro: >> > https://syzkaller.appspot.com/x/repro.syz?x=16587f8c90 >> >> It seems that syzkaller is generating an schedule with too small >> intervals (3ns in this case) which causes a hrtimer busy-loop which >> starves other kernel threads. >> >> We could put some limits on the interval when running in software mode, >> but I don't like this too much, because we are talking about users with >> CAP_NET_ADMIN and they have easier ways to do bad things to the system. > > Hi Vinicius, > > Could you explain why you don't like the argument if it's for CAP_NET_ADMIN? > Good code should check arguments regardless I think and it's useful to > protect root from, say, programming bugs rather than kill the machine > on any bug and misconfiguration. What am I missing? I admit that I am on the fence on that argument: do not let even root crash the system (the point that my code is crashing the system gives weight to this side) vs. root has great powers, they need to know what they are doing. The argument that I used to convince myself was: root can easily create a bunch of processes and give them the highest priority and do effectively the same thing as this issue, so I went with a the "they need to know what they are doing side". A bit more on the specifics here: - Using a small interval size, is only a limitation of the taprio software mode, when using hardware offloads (which I think most users do), any interval size (supported by the hardware) can be used; - Choosing a good lower limit for this seems kind of hard: something below 1us would never work well, I think, but things 1us < x < 100us will depend on the hardware/kernel config/system load, and this is the range includes "useful" values for many systems. Perhaps a middle ground would be to impose a limit based on the link speed, the interval can never be smaller than the time it takes to send the minimum ethernet frame (for 1G links this would be ~480ns, should be enough to catch most programming mistakes). I am going to add this and see how it looks like. Sorry for the brain dump :-) > > Also are we talking about CAP_NET_ADMIN in a user ns as well > (effectively nobody)? Just checked, we are talking about CAP_NET_ADMIN in user namespace as well. Cheers, -- Vinicius
Re: 回复: INFO: rcu detected stall in tc_modify_qdisc
Hi, "Zhang, Qiang" writes: > > 发件人: linux-kernel-ow...@vger.kernel.org > 代表 syzbot > 发送时间: 2020年7月29日 13:53 > 收件人: da...@davemloft.net; fweis...@gmail.com; j...@mojatatu.com; > j...@resnulli.us; linux-kernel@vger.kernel.org; mi...@kernel.org; > net...@vger.kernel.org; syzkaller-b...@googlegroups.com; t...@linutronix.de; > vinicius.go...@intel.com; xiyou.wangc...@gmail.com > 主题: INFO: rcu detected stall in tc_modify_qdisc > > Hello, > > syzbot found the following issue on: > > HEAD commit:181964e6 fix a braino in cmsghdr_from_user_compat_to_kern() > git tree: net > console output: https://syzkaller.appspot.com/x/log.txt?x=12925e3890 > kernel config: https://syzkaller.appspot.com/x/.config?x=f87a5e4232fdb267 > dashboard link: https://syzkaller.appspot.com/bug?extid=9f78d5c664a8c33f4cce > compiler: gcc (GCC) 10.1.0-syz 20200507 > syz repro: > https://syzkaller.appspot.com/x/repro.syz?x=16587f8c90 It seems that syzkaller is generating an schedule with too small intervals (3ns in this case) which causes a hrtimer busy-loop which starves other kernel threads. We could put some limits on the interval when running in software mode, but I don't like this too much, because we are talking about users with CAP_NET_ADMIN and they have easier ways to do bad things to the system. Cheers, -- Vinicius
Re: [net-next RFC PATCH 00/13] net: hsr: Add PRP driver
Murali Karicheri writes:
> Hi Vinicius,
>
> On 5/21/20 1:31 PM, Vinicius Costa Gomes wrote:
>> Murali Karicheri writes:
>>
> Snip-
>
>>>- prefix all common code with hsr_prp
>>>- net/hsr -> renamed to net/hsr-prp
>>>- All common struct types, constants, functions renamed with
>>> hsr{HSR}_prp{PRP} prefix.
>>
>> I don't really like these prefixes, I am thinking of when support for
>> IEEE 802.1CB is added, do we rename this to "hsr_prp_frer"?
>>
>> And it gets even more complicated, and using 802.1CB you can configure
>> the tagging method and the stream identification function so a system
>> can interoperate in a HSR or PRP network.
>>
>> So, I see this as different methods of achieving the same result, which
>> makes me think that the different "methods/types" (HSR and PRP in your
>> case) should be basically different implementations of a "struct
>> hsr_ops" interface. With this hsr_ops something like this:
>>
>> struct hsr_ops {
>>int (*handle_frame)()
>>int (*add_port)()
>>int (*remove_port)()
>>int (*setup)()
>>void (*teardown)()
>> };
>>
>
> Thanks for your response!
>
> I agree with you that the prefix renaming is ugly. However I wasn't
> sure if it is okay to use a hsr prefixed code to handle PRP as
> well as it may not be intuitive to anyone investigating the code. For
> the same reason, handling 802.1CB specifc functions using the hsr_
> prefixed code. If that is okay, then patch 1-6 are unnecessary. We could
> also add some documentation at the top of the file to indicate that
> both hsr and prp are implemented in the code or something like that.
> BTW, I need to investigate more into 802.1CB and this was not known
> when I developed this code few years ago.
I think for now it's better to make it clear how similar PRP and HSR
are.
As for the renaming, I am afraid that this boat has sailed, as the
netlink API already uses HSR_ and it's better to reuse that than create
a new family for, at least conceptually, the same thing (PRP and
802.1CB). And this is important bit, the userspace API.
And even for 802.1CB using name "High-availability Seamless Redudancy"
is as good as any, if very pompous.
>
> Main difference between HSR and PRP is how they handle the protocol tag
> or rct and create or handle the protocol specific part in the frame.
> For that part, we should be able to define ops() like you have
> suggested, instead of doing if check throughout the code. Hope that
> is what you meant by hsr_ops() for this. Again shouldn't we use some
> generic name like proto_ops or red_ops instead of hsr_ops() and assign
> protocol specific implementaion to them? i.e hsr_ or prp_
> or 802.1CB specific functions assigned to the function pointers. For
> now I see handle_frame(), handle_sv_frame, create_frame(),
> create_sv_frame() etc implemented differently (This is currently part of
> patch 11 & 12). So something like
>
> struct proto_ops {
> int (*handle_frame)();
> int (*create_frame)();
> int (*handle_sv_frame)();
> int (*create_sv_frame)();
> };
That's it. That was the idea I was trying to communicate :-)
>
> and call dev->proto_ops->handle_frame() to process a frame from the
> main hook. proto_ops gets initialized to of the set if implementation
> at device or interface creation in hsr_dev_finalize().
>
>>>
>>> Please review this and provide me feedback so that I can work to
>>> incorporate them and send a formal patch series for this. As this
>>> series impacts user space, I am not sure if this is the right
>>> approach to introduce a new definitions and obsolete the old
>>> API definitions for HSR. The current approach is choosen
>>> to avoid redundant code in iproute2 and in the netlink driver
>>> code (hsr_netlink.c). Other approach we discussed internally was
>>> to Keep the HSR prefix in the user space and kernel code, but
>>> live with the redundant code in the iproute2 and hsr netlink
>>> code. Would like to hear from you what is the best way to add
>>> this feature to networking core. If there is any other
>>> alternative approach possible, I would like to hear about the
>>> same.
>>
>> Why redudant code is needed in the netlink parts and in iproute2 when
>> keeping the hsr prefix?
>
> May be this is due to the specific implementation that I chose.
> Currently I have separate netlink socket for HSR and PRP which may
> be an overkill
Re: [net-next RFC PATCH 00/13] net: hsr: Add PRP driver
Hi Vladimir, Vladimir Oltean writes: >> >> I don't really like these prefixes, I am thinking of when support for >> IEEE 802.1CB is added, do we rename this to "hsr_prp_frer"? >> >> And it gets even more complicated, and using 802.1CB you can configure >> the tagging method and the stream identification function so a system >> can interoperate in a HSR or PRP network. >> > > Is it a given that 802.1CB in Linux should be implemented using an hsr > upper device? What I was trying to express is the idea of using "hsr" as the directory name/prefix for all the features that deal with frame replication for reliability, including 802.1CB. At least until we find a better name. > 802.1CB is _much_ more flexible than both HSR and PRP. You can have > more than 2 ports, you can have per-stream rules (each stream has its > own sequence number), and those rules can identify the source, the > destination, or both the source and the destination. Same understanding here. Cheers, -- Vinicius
Re: [net-next RFC PATCH 00/13] net: hsr: Add PRP driver
; hsr-prp}/hsr_netlink.h (58%)
> rename net/{hsr/hsr_debugfs.c => hsr-prp/hsr_prp_debugfs.c} (52%)
> create mode 100644 net/hsr-prp/hsr_prp_device.c
> create mode 100644 net/hsr-prp/hsr_prp_device.h
> create mode 100644 net/hsr-prp/hsr_prp_forward.c
> rename net/{hsr/hsr_forward.h => hsr-prp/hsr_prp_forward.h} (50%)
> rename net/{hsr/hsr_framereg.c => hsr-prp/hsr_prp_framereg.c} (56%)
> create mode 100644 net/hsr-prp/hsr_prp_framereg.h
> create mode 100644 net/hsr-prp/hsr_prp_main.c
> create mode 100644 net/hsr-prp/hsr_prp_main.h
> create mode 100644 net/hsr-prp/hsr_prp_netlink.c
> create mode 100644 net/hsr-prp/hsr_prp_netlink.h
> create mode 100644 net/hsr-prp/hsr_prp_slave.c
> create mode 100644 net/hsr-prp/hsr_prp_slave.h
> create mode 100644 net/hsr-prp/prp_netlink.c
> create mode 100644 net/hsr-prp/prp_netlink.h
> delete mode 100644 net/hsr/Kconfig
> delete mode 100644 net/hsr/Makefile
> delete mode 100644 net/hsr/hsr_device.c
> delete mode 100644 net/hsr/hsr_device.h
> delete mode 100644 net/hsr/hsr_forward.c
> delete mode 100644 net/hsr/hsr_framereg.h
> delete mode 100644 net/hsr/hsr_main.c
> delete mode 100644 net/hsr/hsr_main.h
> delete mode 100644 net/hsr/hsr_netlink.c
> delete mode 100644 net/hsr/hsr_slave.c
> delete mode 100644 net/hsr/hsr_slave.h
>
> --
> 2.17.1
>
--
Vinicius
Re: [EXT] Re: [v1,net-next, 1/2] ethtool: add setting frame preemption of traffic classes
Hi Murali, Murali Karicheri writes: > Any progress on your side for a patch for the support? > Sorry for the delay, things got a bit crazy here for some time. I have a RFC-quality series that I am finishing testing, I'll try to post it this week. > I have posted my EST offload series for AM65x CPSW to netdev list today > at > > https://marc.info/?l=linux-netdev=158937640015582=2 > https://marc.info/?l=linux-netdev=158937639515579=2 > https://marc.info/?l=linux-netdev=158937638315573=2 I'll try to have a look. > > Next on my list of things to do is the IET FPE support for which I need > to have ethtool interface to allow configuring the express/preemptible > queues and feature enable/disable. Currently I am using a ethtool > priv-flags and some defaults. If you can post a patch, I will be able > to integrate and test it on AM65x CPSW driver and provide my comments/ > Tested-by: Understood. Thanks. -- Vinicius
Re: [PATCH v1 net-next 3/3] net: dsa: felix: add support Credit Based Shaper(CBS) for hardware offload
Xiaoliang Yang writes:
> VSC9959 hardware support the Credit Based Shaper(CBS) which part
> of the IEEE-802.1Qav. This patch support sch_cbs set for VSC9959.
>
> Signed-off-by: Xiaoliang Yang
> ---
> drivers/net/dsa/ocelot/felix_vsc9959.c | 52 +-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c
> b/drivers/net/dsa/ocelot/felix_vsc9959.c
> index ccbd875c7a47..d8d1657ee8ba 100644
> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
> @@ -208,7 +208,7 @@ static const u32 vsc9959_qsys_regmap[] = {
> REG(QSYS_QMAXSDU_CFG_6, 0x00f62c),
> REG(QSYS_QMAXSDU_CFG_7, 0x00f648),
> REG(QSYS_PREEMPTION_CFG,0x00f664),
> - REG_RESERVED(QSYS_CIR_CFG),
> + REG(QSYS_CIR_CFG, 0x00),
> REG(QSYS_EIR_CFG, 0x04),
> REG(QSYS_SE_CFG,0x08),
> REG(QSYS_SE_DWRR_CFG, 0x0c),
> @@ -1354,6 +1354,54 @@ static int vsc9959_qos_port_tas_set(struct ocelot
> *ocelot, int port,
> return ret;
> }
>
> +int vsc9959_qos_port_cbs_set(struct dsa_switch *ds, int port,
> + struct tc_cbs_qopt_offload *cbs_qopt)
> +{
> + struct ocelot *ocelot = ds->priv;
> + int port_ix = port * 8 + cbs_qopt->queue;
> + u32 cbs = 0;
> + u32 cir = 0;
> +
> + if (cbs_qopt->queue >= ds->num_tx_queues)
> + return -EINVAL;
> +
> + if (!cbs_qopt->enable) {
> + ocelot_write_gix(ocelot, QSYS_CIR_CFG_CIR_RATE(0) |
> + QSYS_CIR_CFG_CIR_BURST(0),
> + QSYS_CIR_CFG, port_ix);
> +
> + ocelot_rmw_gix(ocelot, 0, QSYS_SE_CFG_SE_AVB_ENA,
> +QSYS_SE_CFG, port_ix);
> +
> + return 0;
> + }
> +
> + /* Rate unit is 100 kbps */
> + cir = DIV_ROUND_UP(cbs_qopt->idleslope, 100);
> + cir = (cir ? cir : 1);
> + cir = min_t(u32, GENMASK(14, 0), cir);
Please rename 'cir' to "rate" or "idleslope".
Also consider using clamp_t here and below (I just found out about it).
> + /* Burst unit is 4kB */
> + cbs = DIV_ROUND_UP(cbs_qopt->hicredit, 4096);
> + /* Avoid using zero burst size */
> + cbs = (cbs ? cbs : 1);
> + cbs = min_t(u32, GENMASK(5, 0), cbs);
And please(!) rename 'cbs' to "burst" or "hicredit". Re-using the name
"cbs" with a completely different meaning here is confusing.
> + ocelot_write_gix(ocelot,
> + QSYS_CIR_CFG_CIR_RATE(cir) |
> + QSYS_CIR_CFG_CIR_BURST(cbs),
> + QSYS_CIR_CFG,
> + port_ix);
> +
> + ocelot_rmw_gix(ocelot,
> +QSYS_SE_CFG_SE_FRM_MODE(0) |
> +QSYS_SE_CFG_SE_AVB_ENA,
> +QSYS_SE_CFG_SE_AVB_ENA |
> +QSYS_SE_CFG_SE_FRM_MODE_M,
> +QSYS_SE_CFG,
> +port_ix);
> +
> + return 0;
> +}
> +
> static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port,
>enum tc_setup_type type,
>void *type_data)
> @@ -1363,6 +1411,8 @@ static int vsc9959_port_setup_tc(struct dsa_switch *ds,
> int port,
> switch (type) {
> case TC_SETUP_QDISC_TAPRIO:
> return vsc9959_qos_port_tas_set(ocelot, port, type_data);
> + case TC_SETUP_QDISC_CBS:
> + return vsc9959_qos_port_cbs_set(ds, port, type_data);
> default:
> return -EOPNOTSUPP;
> }
> --
> 2.17.1
>
--
Vinicius
Re: [net-next PATCH] net: hsr: fix incorrect type usage for protocol variable
Hi Murali, Murali Karicheri writes: > Fix following sparse checker warning:- > > net/hsr/hsr_slave.c:38:18: warning: incorrect type in assignment (different > base types) > net/hsr/hsr_slave.c:38:18:expected unsigned short [unsigned] [usertype] > protocol > net/hsr/hsr_slave.c:38:18:got restricted __be16 [usertype] h_proto > net/hsr/hsr_slave.c:39:25: warning: restricted __be16 degrades to integer > net/hsr/hsr_slave.c:39:57: warning: restricted __be16 degrades to integer > > Signed-off-by: Murali Karicheri > --- I think this patch should go via the net tree, as it is a warning fix. Anyway... Acked-by: Vinicius Costa Gomes -- Vinicius
Re: [PATCH] net: sched: taprio: fix -Wmissing-prototypes warnings
Hi, Yi Wang writes: > We get one warnings when build kernel W=1: > net/sched/sch_taprio.c:1155:6: warning: no previous prototype for > ‘taprio_offload_config_changed’ [-Wmissing-prototypes] > > Make the function static to fix this. > > Signed-off-by: Yi Wang > --- This looks like it should be directed to net-next. When you re-send it for net-next, feel free to add my: Acked-by: Vinicius Costa Gomes Cheers, -- Vinicius
RE: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte
Hi Vladimir, > Looks ok to me, but I have no saying over ethtool API. Actually I don't even > know whom to ask - the output of ./scripts/get_maintainer.pl > net/core/ethtool.c is a bit overwhelming. > To avoid conflicts, there needs to be somebody out of us who takes Eric's > simplification, with Gustavo's Reported-by tag, and the 2 ethtool & taprio > patches to avoid division by zero, and the printing fix, and maybe do the > same in > cbs. Will you be the one? Should I? If you have the cycles to do it, go for it. I would only be able to work on this next week. > > Thanks, > -Vladimir Thanks a lot, -- Vinicius
Re: [PATCH] net: sched: taprio: Fix potential integer overflow in taprio_set_picos_per_byte
Hi,
Vladimir Oltean writes:
> Right. And while we're at it, there's still the potential
> division-by-zero problem which I still don't know how to solve without
> implementing a full-blown __ethtool_get_link_ksettings parser that
> checks against all the possible outputs it can have under the "no
> carrier" condition - see "[RFC PATCH 1/1] phylink: Set speed to
> SPEED_UNKNOWN when there is no PHY connected" for details.
> And there's also a third fix to be made: the netdev_dbg should be made
> to print "speed" instead of "ecmd.base.speed".
For the ksettings part I am thinking on adding something like this to
ethtool.c. Do you think anything is missing (apart from the
documentation)?
->
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 95991e43..d37c80b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -177,6 +177,9 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned long
*dst,
bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
const unsigned long *src);
+u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings
*settings,
+ u32 default_speed);
+
/**
* struct ethtool_ops - optional netdev operations
* @get_drvinfo: Report driver/device information. Should only set the
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6288e69..80e3db3 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -539,6 +539,18 @@ struct ethtool_link_usettings {
} link_modes;
};
+u32 ethtool_link_ksettings_to_speed(const struct ethtool_link_ksettings
*settings,
+ u32 default_speed)
+{
+ if (settings->base.speed == SPEED_UNKNOWN)
+ return default_speed;
+
+ if (settings->base.speed == 0)
+ return default_speed;
+
+ return settings->base.speed;
+}
+
/* Internal kernel helper to query a device ethtool_link_settings. */
int __ethtool_get_link_ksettings(struct net_device *dev,
struct ethtool_link_ksettings *link_ksettings)
Re: [PATCH net-next] taprio: remove unused variable 'entry_list_policy'
Hi,
David Miller writes:
> From: YueHaibing
> Date: Thu, 8 Aug 2019 22:26:23 +0800
>
>> net/sched/sch_taprio.c:680:32: warning:
>> entry_list_policy defined but not used [-Wunused-const-variable=]
>>
>> It is not used since commit a3d43c0d56f1 ("taprio: Add
>> support adding an admin schedule")
>>
>> Reported-by: Hulk Robot
>> Signed-off-by: YueHaibing
>
> This is probably unintentional and a bug, we should be using that
> policy value to validate that the sched list is indeed a nested
> attribute.
Removing this policy should be fine.
One of the points of commit (as explained in the commit message)
a3d43c0d56f1 ("taprio: Add support adding an admin schedule") is that it
removes support (it now returns "not supported") for schedules using the
TCA_TAPRIO_ATTR_SCHED_SINGLE_ENTRY attribute (which were never used),
the parsing of those types of schedules was the only user of this
policy.
>
> I'm not applying this without at least a better and clear commit
> message explaining why we shouldn't be using this policy any more.
YueHaibing may use the text above in the commit message of a new spin of
this patch if you think it's clear enough.
Cheers,
--
Vinicius
Re: [PATCH] net: sched: sch_taprio: fix memleak in error path for sched list parse
Ivan Khoronzhuk writes:
> In case off error, all entries should be freed from the sched list
> before deleting it. For simplicity use rcu way.
>
> Fixes: 5a781ccbd19e46 ("tc: Add support for configuring the taprio scheduler")
> Signed-off-by: Ivan Khoronzhuk
> ---
Acked-by: Vinicius Costa Gomes
Re: [RFC net-next 1/5] net: stmmac: introduce IEEE 802.1Qbv configuration functionalities
Hi,
Voon Weifeng writes:
> From: Ong Boon Leong
>
> IEEE 802.1Qbv Enhancements for Scheduled Traffics (EST) is available in
> EQoS ver5.xx. The change adds basic EST functionalities:
>
> a) EST initialization with hardware capabilities detection.
> b) Setting Gate Control List (GCL), i.e. gate open/close & time intervals,
>and all GC Related Registers (GCRR), e.g., base time (BTR), cycle time
>(CTR), time extension (TER) and GC List Length (LLR).
> c) Setting time interval left shift (TILS), PTP time offset (PTOV) and
>current time offset (CTOV).
> d) Enable/disable EST.
> e) Getting TSN hardware capabilities.
> f) Getting Gate Control configuration either from driver data store or
>hardware.
>
> We extend the main driver logic to include basic TSN capability discovery,
> and setup. We also add EST feature enable/disable control.
>
> Reviewed-by: Chuah Kim Tatt
> Reviewed-by: Voon Weifeng
> Reviewed-by: Kweh Hock Leong
> Signed-off-by: Ong Boon Leong
> Signed-off-by: Voon Weifeng
> ---
> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
> drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c | 790
> ++
> drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.h | 173 +
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 13 +
> drivers/net/ethernet/stmicro/stmmac/hwif.h| 52 ++
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 46 ++
> include/linux/stmmac.h| 1 +
> 7 files changed, 1076 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.h
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
> b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index c59926d96bcc..76fb36cb4da7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o
> ring_mode.o\
> mmc_core.o stmmac_hwtstamp.o stmmac_ptp.o dwmac4_descs.o \
> dwmac4_dma.o dwmac4_lib.o dwmac4_core.o dwmac5.o hwif.o \
> stmmac_tc.o dwxgmac2_core.o dwxgmac2_dma.o dwxgmac2_descs.o \
> - $(stmmac-y)
> + dw_tsn_lib.o $(stmmac-y)
>
> stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
> b/drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
> new file mode 100644
> index ..cba27c604cb1
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
> @@ -0,0 +1,790 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright (c) 2019, Intel Corporation.
> + * dw_tsn_lib.c: DW EQoS v5.00 TSN capabilities
> + */
> +
> +#include "dwmac4.h"
> +#include "dwmac5.h"
> +#include "dw_tsn_lib.h"
> +
> +static struct tsn_hw_cap dw_tsn_hwcap;
> +static bool dw_tsn_feat_en[TSN_FEAT_ID_MAX];
> +static unsigned int dw_tsn_hwtunable[TSN_HWTUNA_MAX];
> +static struct est_gc_config dw_est_gc_config;
If it's at all possible to have more than one of these devices in a
system, this should be moved to a per-device structure. That
mac_device_info struct perhaps?
> +
> +static unsigned int est_get_gcl_depth(unsigned int hw_cap)
> +{
> + unsigned int estdep = (hw_cap & GMAC_HW_FEAT_ESTDEP)
> + >> GMAC_HW_FEAT_ESTDEP_SHIFT;
> + unsigned int depth;
> +
> + switch (estdep) {
> + case 1:
> + depth = 64;
> + break;
> + case 2:
> + depth = 128;
> + break;
> + case 3:
> + depth = 256;
> + break;
> + case 4:
> + depth = 512;
> + break;
> + case 5:
> + depth = 1024;
> + break;
> + default:
> + depth = 0;
> + }
> +
> + return depth;
> +}
> +
> +static unsigned int est_get_ti_width(unsigned int hw_cap)
> +{
> + unsigned int estwid = (hw_cap & GMAC_HW_FEAT_ESTWID)
> + >> GMAC_HW_FEAT_ESTWID_SHIFT;
> + unsigned int width;
> +
> + switch (estwid) {
> + case 1:
> + width = 16;
> + break;
> + case 2:
> + width = 20;
> + break;
> + case 3:
> + width = 24;
> + break;
> + default:
> + width = 0;
> + }
> +
> + return width;
> +}
> +
> +static int est_poll_srwo(void *ioaddr)
> +{
> + /* Poll until the EST GCL Control[SRWO] bit clears.
> + * Total wait = 12 x 50ms ~= 0.6s.
> + */
> + unsigned int retries = 12;
> + unsigned int value;
> +
> + do {
> + value = TSN_RD32(ioaddr + MTL_EST_GCL_CTRL);
> + if (!(value & MTL_EST_GCL_CTRL_SRWO))
> + return 0;
> + msleep(50);
> + } while (--retries);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int est_set_gcl_addr(void *ioaddr,
RE: [PATCH] net: tsn: add an netlink interface between kernel and application layer
Hi Po Liu, PO LIU writes: > Hi Vinicius, > > Thank you very much for your feedback. > > I know the CBS is used to be most important part of AVB. And qdiscs is good > tool to configure qos. > > But as you know, the TSN family is a cluster of protocols and much extending > the AVB. The protocols have different functionalities and they may have more > than hundred parameters. For example NXP ls1028a support Qbv/Qci/Qbu/Qav and > also the 8021CB (not included in this patch yet). > > Some protocols target to configure the traffic class(like Qav CBS). > Some to config the port(like Qbv). But some for the whole ethernet > controller(like Qci, the control entries for the whole controller, > which input ports and which output ports). Reading your email, now I understand your point a little better. You are interested in multi-port devices. I admit that I am not too familiar with how multi-port devices are exposed in Linux, I was only focused on the end-station use cases, until now. > > So I do think all the TSN configuration should not mix in the ethernet > driver itself. I mean the driver should separate a xxx_tsn.c(for I210, > may igb_tsn.c) to maintain the tsn operations. > As far as using qdiscs or the interface of generic netlink. I think > both could configuring the TSN protocols interface layer. Just what I > provided the patch net/tsn/genl_tsn.c. But I do believe it is better > using a standalone TSN middle layer to maintain the TSN capability > ports. Because the TSN ports include not only the end station and also > the switch. LS1028 is such a kind of device. I think this is the "interesting" part of the discussion. From my point of view the question now is: "We already have an acceptable way to expoose TSN features for end stations. What can we do for multi-port devices?" What are the options here? From a quick look, it seems that extending switchdev is a possible solution. What else? Thinking a little more, if all the ports have netdevices associated with them, then it could be that exposing those features via qdiscs could be considered still. Perhaps taking a look at how tc-flower offloading is done can give some ideas. And about the process, usually when a new interface is proposed, the patches are directed to net-next and have the RFC tag, so the readers (and their tools) know what to expect. > > And your advises are precious for us. Let's make out an easy and > flexible interface for TSN. > > Br, > Po Liu > Cheers, -- Vinicius
Re: [PATCH] net: tsn: add an netlink interface between kernel and application layer
Hi, PO LIU writes: > This patch provids netlink method to configure the TSN protocols hardwares. > TSN guaranteed packet transport with bounded low latency, low packet delay > variation, and low packet loss by hardware and software methods. I don't think having another way to configure TSN features is a good idea. We already have the CBS/ETF/taprio family of qdiscs, that provide (or will in the near future, more on this later) a way to configure the hardware. A little background on the choice of qdiscs as an interface (and why we came to believe they are a good abstraction), they already provide a way to map packets into traffic classes (it isn't clear in our proposal how you do that, but I think you are using something like mqprio), they provide a neat way to "compose" (by installing one under another), they already have a user facing API with various counters, and very importantly for TSN they have mecanisms to offload some of their work to the hardware. I suggest is for you to take a look at how CBS offloading was implemented for the Intel i210: https://patchwork.ozlabs.org/cover/824626/ Patches 4 and 5 should be the interesting ones. I think you can use them as inspiration for enabling CBS offload in your driver. If you did take a look at those patches (and the current work that has been upstreamed), my question then becomes, what are the reasons that it might not work for your use cases? > > The three basic components of TSN are: > > 1. Time synchronization: This was implement by 8021AS which base on the >IEEE1588 precision Time Protocol. This is configured by the other way >in kernel. >8021AS not included in this patch. > > 2. Scheduling and traffic shaping and per-stream filter policing: >This patch support Qbv/Qci. I am working on a proposal for the API for Qbv (and Qbu) offloading using taprio. I should send it soon-ish. Your feedback would be very welcome. Also, how to expose in the qdiscs the per-stream filtering and policing parts (Qci) is something that I don't know how to do right now, any suggestions would be nice. In short, take a look at what's there and see what's missing for the stuff that you care about, then we can work on that. > > 3. Selection of communication paths: >This patch not support the pure software only TSN protocols(like Qcc) >but hardware related configuration. > > TSN Protocols supports by this patch: Qbv/Qci/Qbu/Credit-base Shaper(Qav). > This patch verified on NXP ls1028ardb board. > > Will add more protocols in the future. > > Signed-off-by: Po Liu Cheers, -- Vinicius
Re: [PATCH] igb: Fix format with line continuation whitespace
Joe Perches writes: > The line continuation unintentionally adds whitespace so > instead use a coalesced format to remove the whitespace. > > Miscellanea: > > o Use a more typical style for ternaries and arguments > for this logging message > > Signed-off-by: Joe Perches Acked-by: Vinicius Costa Gomes Cheers, -- Vinicius
Re: [PATCH] igb: Fix format with line continuation whitespace
Joe Perches writes: > The line continuation unintentionally adds whitespace so > instead use a coalesced format to remove the whitespace. > > Miscellanea: > > o Use a more typical style for ternaries and arguments > for this logging message > > Signed-off-by: Joe Perches Acked-by: Vinicius Costa Gomes Cheers, -- Vinicius
Re: [PATCH 11/11] perf tools: Stop fallbacking to kallsyms for vdso symbols lookup
Hi Jirka, Jiri Olsa writes: > On Fri, Oct 26, 2018 at 04:19:52PM -0700, Vinicius Costa Gomes wrote: >> Hi, >> >> Adrian Hunter writes: >> >> > On 18/10/18 1:55 AM, Arnaldo Carvalho de Melo wrote: >> >> From: Arnaldo Carvalho de Melo >> >> >> >> David reports that: >> >> >> >> >> >> Perf has this hack where it uses the kernel symbol map as a backup when >> >> a symbol can't be found in the user's symbol table(s). >> > >> > I don't think this is a complete fix because it exposes new problems. >> >> This commit broke function name resolution for 'perf record -g' for me. >> >> What I mean is, with this commit applied: >> >> $ ./tools/perf/perf record -g -- sleep 1 >> >> $ ./tools/perf/perf report >> >> 'perf report' doesn't seem to be able to show the function names of the >> trace. >> >> If I revert this commit, function names are resolved fine. > > that commit just showed up some places where we have the > ip resolve wrong.. would attached patch fix it for you? Thank you for your patch. I can some difference in the output, but I wouldn't say that it's fixed. Here are some samples, if it's useful somehow: https://gist.github.com/vcgomes/985626705e0968b973e426964f86a4b0 The "ping" tests were done running $ sudo ./tools/perf/perf record -g -- ping -f -c 1000 127.0.0.1 And the "sleep" tests were done running $ sudo ./tools/perf/perf record -g -- /bin/sleep 1 -- Vinicius
Re: [PATCH 11/11] perf tools: Stop fallbacking to kallsyms for vdso symbols lookup
Hi Jirka, Jiri Olsa writes: > On Fri, Oct 26, 2018 at 04:19:52PM -0700, Vinicius Costa Gomes wrote: >> Hi, >> >> Adrian Hunter writes: >> >> > On 18/10/18 1:55 AM, Arnaldo Carvalho de Melo wrote: >> >> From: Arnaldo Carvalho de Melo >> >> >> >> David reports that: >> >> >> >> >> >> Perf has this hack where it uses the kernel symbol map as a backup when >> >> a symbol can't be found in the user's symbol table(s). >> > >> > I don't think this is a complete fix because it exposes new problems. >> >> This commit broke function name resolution for 'perf record -g' for me. >> >> What I mean is, with this commit applied: >> >> $ ./tools/perf/perf record -g -- sleep 1 >> >> $ ./tools/perf/perf report >> >> 'perf report' doesn't seem to be able to show the function names of the >> trace. >> >> If I revert this commit, function names are resolved fine. > > that commit just showed up some places where we have the > ip resolve wrong.. would attached patch fix it for you? Thank you for your patch. I can some difference in the output, but I wouldn't say that it's fixed. Here are some samples, if it's useful somehow: https://gist.github.com/vcgomes/985626705e0968b973e426964f86a4b0 The "ping" tests were done running $ sudo ./tools/perf/perf record -g -- ping -f -c 1000 127.0.0.1 And the "sleep" tests were done running $ sudo ./tools/perf/perf record -g -- /bin/sleep 1 -- Vinicius
Re: [PATCH 11/11] perf tools: Stop fallbacking to kallsyms for vdso symbols lookup
Hi,
Adrian Hunter writes:
> On 18/10/18 1:55 AM, Arnaldo Carvalho de Melo wrote:
>> From: Arnaldo Carvalho de Melo
>>
>> David reports that:
>>
>>
>> Perf has this hack where it uses the kernel symbol map as a backup when
>> a symbol can't be found in the user's symbol table(s).
>
> I don't think this is a complete fix because it exposes new problems.
This commit broke function name resolution for 'perf record -g' for me.
What I mean is, with this commit applied:
$ ./tools/perf/perf record -g -- sleep 1
$ ./tools/perf/perf report
'perf report' doesn't seem to be able to show the function names of the
trace.
If I revert this commit, function names are resolved fine.
> This code caters for branches from kernel space to user space and vice
> versa. That is, since there is only one cpumode so it is certain to be
> wrong for either 'ip' or 'addr' when they are not both in the kernel
> or both in userspace.
>
>>
>>
>> Cc: Adrian Hunter
>> Cc: David Ahern
>> Cc: Jiri Olsa
>> Cc: Namhyung Kim
>> Cc: Wang Nan
>> Link: https://lkml.kernel.org/n/tip-cs7skq9pp0kjypiju6o7t...@git.kernel.org
>> Signed-off-by: Arnaldo Carvalho de Melo
>> ---
>> tools/perf/util/event.c | 21 ++---
>> 1 file changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
>> index 0988eb3b844b..bc646185f8d9 100644
>> --- a/tools/perf/util/event.c
>> +++ b/tools/perf/util/event.c
>> @@ -1561,26 +1561,9 @@ struct map *thread__find_map(struct thread *thread,
>> u8 cpumode, u64 addr,
>>
>> return NULL;
>> }
>> -try_again:
>> +
>> al->map = map_groups__find(mg, al->addr);
>> -if (al->map == NULL) {
>> -/*
>> - * If this is outside of all known maps, and is a negative
>> - * address, try to look it up in the kernel dso, as it might be
>> - * a vsyscall or vdso (which executes in user-mode).
>> - *
>> - * XXX This is nasty, we should have a symbol list in the
>> - * "[vdso]" dso, but for now lets use the old trick of looking
>> - * in the whole kernel symbol list.
>> - */
>> -if (cpumode == PERF_RECORD_MISC_USER && machine &&
>> -mg != >kmaps &&
>> -machine__kernel_ip(machine, al->addr)) {
>> - mg = >kmaps;
>> -load_map = true;
>> -goto try_again;
>> -}
>> -} else {
>> +if (al->map != NULL) {
>> /*
>> * Kernel maps might be changed when loading symbols so loading
>> * must be done prior to using kernel maps.
>>
--
Vinicius
Re: [PATCH 11/11] perf tools: Stop fallbacking to kallsyms for vdso symbols lookup
Hi,
Adrian Hunter writes:
> On 18/10/18 1:55 AM, Arnaldo Carvalho de Melo wrote:
>> From: Arnaldo Carvalho de Melo
>>
>> David reports that:
>>
>>
>> Perf has this hack where it uses the kernel symbol map as a backup when
>> a symbol can't be found in the user's symbol table(s).
>
> I don't think this is a complete fix because it exposes new problems.
This commit broke function name resolution for 'perf record -g' for me.
What I mean is, with this commit applied:
$ ./tools/perf/perf record -g -- sleep 1
$ ./tools/perf/perf report
'perf report' doesn't seem to be able to show the function names of the
trace.
If I revert this commit, function names are resolved fine.
> This code caters for branches from kernel space to user space and vice
> versa. That is, since there is only one cpumode so it is certain to be
> wrong for either 'ip' or 'addr' when they are not both in the kernel
> or both in userspace.
>
>>
>>
>> Cc: Adrian Hunter
>> Cc: David Ahern
>> Cc: Jiri Olsa
>> Cc: Namhyung Kim
>> Cc: Wang Nan
>> Link: https://lkml.kernel.org/n/tip-cs7skq9pp0kjypiju6o7t...@git.kernel.org
>> Signed-off-by: Arnaldo Carvalho de Melo
>> ---
>> tools/perf/util/event.c | 21 ++---
>> 1 file changed, 2 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
>> index 0988eb3b844b..bc646185f8d9 100644
>> --- a/tools/perf/util/event.c
>> +++ b/tools/perf/util/event.c
>> @@ -1561,26 +1561,9 @@ struct map *thread__find_map(struct thread *thread,
>> u8 cpumode, u64 addr,
>>
>> return NULL;
>> }
>> -try_again:
>> +
>> al->map = map_groups__find(mg, al->addr);
>> -if (al->map == NULL) {
>> -/*
>> - * If this is outside of all known maps, and is a negative
>> - * address, try to look it up in the kernel dso, as it might be
>> - * a vsyscall or vdso (which executes in user-mode).
>> - *
>> - * XXX This is nasty, we should have a symbol list in the
>> - * "[vdso]" dso, but for now lets use the old trick of looking
>> - * in the whole kernel symbol list.
>> - */
>> -if (cpumode == PERF_RECORD_MISC_USER && machine &&
>> -mg != >kmaps &&
>> -machine__kernel_ip(machine, al->addr)) {
>> - mg = >kmaps;
>> -load_map = true;
>> -goto try_again;
>> -}
>> -} else {
>> +if (al->map != NULL) {
>> /*
>> * Kernel maps might be changed when loading symbols so loading
>> * must be done prior to using kernel maps.
>>
--
Vinicius
Re: [PATCH RFC V1 net-next 0/6] Time based packet transmission
Hi David, David Miller <da...@davemloft.net> writes: > From: Richard Cochran <rcoch...@linutronix.de> > Date: Mon, 18 Sep 2017 09:41:15 +0200 > >> - The driver does not handle out of order packets. If user space >> sends a packet with an earlier Tx time, then the code should stop >> the queue, reshuffle the descriptors accordingly, and then >> restart the queue. > > The user should simply be not allowed to do this. > > Once the packet is in the device queue, that's it. You cannot insert > a new packet to be transmitted before an already hw queued packet, > period. > > Any out of order request should be rejected with an error. Just to clarify, I agree that after after the packet is enqueued to the HW, there's no going back, in another words, we must never enqueue anything to the HW with a timestamp earlier than the last enqueued packet. But re-ordering packets at the Qdisc level is, I think, necessary: two applications (one (A) with period of 50us and the other (B) of 100us), if it happens that (B) enqueues its packet before (A), I think, we would have a problem. The problem is deciding for how long we should keep packets in the Qdisc queue. In the implementation we are working on, this is left for the user to decide. Or do you have a reason for not doing *any* kind of re-ordering? > > I'd say the same is true for requests to send packets timed > in the past. +1 Cheers, -- Vinicius
Re: [PATCH RFC V1 net-next 0/6] Time based packet transmission
Hi David, David Miller writes: > From: Richard Cochran > Date: Mon, 18 Sep 2017 09:41:15 +0200 > >> - The driver does not handle out of order packets. If user space >> sends a packet with an earlier Tx time, then the code should stop >> the queue, reshuffle the descriptors accordingly, and then >> restart the queue. > > The user should simply be not allowed to do this. > > Once the packet is in the device queue, that's it. You cannot insert > a new packet to be transmitted before an already hw queued packet, > period. > > Any out of order request should be rejected with an error. Just to clarify, I agree that after after the packet is enqueued to the HW, there's no going back, in another words, we must never enqueue anything to the HW with a timestamp earlier than the last enqueued packet. But re-ordering packets at the Qdisc level is, I think, necessary: two applications (one (A) with period of 50us and the other (B) of 100us), if it happens that (B) enqueues its packet before (A), I think, we would have a problem. The problem is deciding for how long we should keep packets in the Qdisc queue. In the implementation we are working on, this is left for the user to decide. Or do you have a reason for not doing *any* kind of re-ordering? > > I'd say the same is true for requests to send packets timed > in the past. +1 Cheers, -- Vinicius
Re: [PATCH v5 BlueZ 4/4] Bluetooth: Handle Slave Connection Interval Range AD
Hi Felipe,
"Felipe F. Tonello" <e...@felipetonello.com> writes:
> The Slave Connection Interval Range data type contains the Peripheral's
> preferred connection interval range, for all logical connections.
>
> It is useful to parse it in the Kernel so there is no multiple calls to
> MGMT interface to update the device connection parameters and subsequent
> connection command call to this device will use proper connection
> parameters. This saves context-switches and eliminates user-space to
> update the connection parameters each time a device is found or
> bluetoothd is restarted and so on. Also, there is no need for the
> user-space to know care about it because if the slave device wishes to
> persist with these parameters, it should use the L2CAP connection
> parameters upade request upon a completed connection.
nitpick: upade -> update
>
> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
> ---
> net/bluetooth/mgmt.c | 53
>
> 1 file changed, 53 insertions(+)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1fba2a03f8ae..ea5d6c85f173 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -7442,6 +7442,46 @@ static bool is_filter_match(struct hci_dev *hdev, s8
> rssi, u8 *eir,
> return true;
> }
>
> +static bool has_eir_slave_conn_int(const u8 *eir_data, u8 eir_len,
> +u16 *min_conn, u16 *max_conn)
> +{
> + u16 len = 0;
> + const u8 EIR_SLAVE_CONN_INT = 0x12; /* Slave Connection Interval Range
> */
> +
> + while (len < eir_len - 1) {
> + u8 field_len = eir_data[0];
> + const u8 *data;
> + u8 data_len;
> +
> + /* Check for the end of EIR */
> + if (field_len == 0)
> + break;
> +
> + len += field_len + 1;
> +
> + /* Do not continue EIR Data parsing if got
> + * incorrect length
> + */
> + if (len > eir_len)
> + break;
> +
> + data = _data[2];
> + data_len = field_len - 1;
> +
> + if (eir_data[1] == EIR_SLAVE_CONN_INT) {
> + if (data_len < 4)
> + break;
> + *min_conn = le16_to_cpu([0]);
> + *max_conn = le16_to_cpu([2]);
> + return true;
> + }
> +
> + eir_data += field_len + 1;
> + }
> +
> + return false;
> +}
> +
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
> u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
> @@ -7449,6 +7489,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
> *bdaddr, u8 link_type,
> char buf[512];
> struct mgmt_ev_device_found *ev = (void *)buf;
> size_t ev_size;
> + struct hci_conn *hcon;
>
> /* Don't send events for a non-kernel initiated discovery. With
>* LE one exception is if we have pend_le_reports > 0 in which
> @@ -7521,6 +7562,18 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
> *bdaddr, u8 link_type,
> ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
> ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>
> + /* Search for Slave Connection Interval AD */
> + hcon = hci_conn_hash_lookup_le(hdev, bdaddr, addr_type);
> + if (hcon) {
> + u16 min_conn_int, max_conn_int;
> +
> + if (has_eir_slave_conn_int(ev->eir, ev->eir_len,
> +_conn_int, _conn_int)) {
> + hcon->le_conn_min_interval = min_conn_int;
> + hcon->le_conn_max_interval = max_conn_int;
> + }
> + }
> +
It's been some time that I looked at this code, so I could be missing
something, but I got the feeling that this part would make more sense if
it was at process_adv_report(), there's even the check for a pending
connection, so no need to redo that here.
Apart from this, the series looks good.
> mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> }
>
> --
> 2.12.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
Re: [PATCH v5 BlueZ 4/4] Bluetooth: Handle Slave Connection Interval Range AD
Hi Felipe,
"Felipe F. Tonello" writes:
> The Slave Connection Interval Range data type contains the Peripheral's
> preferred connection interval range, for all logical connections.
>
> It is useful to parse it in the Kernel so there is no multiple calls to
> MGMT interface to update the device connection parameters and subsequent
> connection command call to this device will use proper connection
> parameters. This saves context-switches and eliminates user-space to
> update the connection parameters each time a device is found or
> bluetoothd is restarted and so on. Also, there is no need for the
> user-space to know care about it because if the slave device wishes to
> persist with these parameters, it should use the L2CAP connection
> parameters upade request upon a completed connection.
nitpick: upade -> update
>
> Signed-off-by: Felipe F. Tonello
> ---
> net/bluetooth/mgmt.c | 53
>
> 1 file changed, 53 insertions(+)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1fba2a03f8ae..ea5d6c85f173 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -7442,6 +7442,46 @@ static bool is_filter_match(struct hci_dev *hdev, s8
> rssi, u8 *eir,
> return true;
> }
>
> +static bool has_eir_slave_conn_int(const u8 *eir_data, u8 eir_len,
> +u16 *min_conn, u16 *max_conn)
> +{
> + u16 len = 0;
> + const u8 EIR_SLAVE_CONN_INT = 0x12; /* Slave Connection Interval Range
> */
> +
> + while (len < eir_len - 1) {
> + u8 field_len = eir_data[0];
> + const u8 *data;
> + u8 data_len;
> +
> + /* Check for the end of EIR */
> + if (field_len == 0)
> + break;
> +
> + len += field_len + 1;
> +
> + /* Do not continue EIR Data parsing if got
> + * incorrect length
> + */
> + if (len > eir_len)
> + break;
> +
> + data = _data[2];
> + data_len = field_len - 1;
> +
> + if (eir_data[1] == EIR_SLAVE_CONN_INT) {
> + if (data_len < 4)
> + break;
> + *min_conn = le16_to_cpu([0]);
> + *max_conn = le16_to_cpu([2]);
> + return true;
> + }
> +
> + eir_data += field_len + 1;
> + }
> +
> + return false;
> +}
> +
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
> u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
> @@ -7449,6 +7489,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
> *bdaddr, u8 link_type,
> char buf[512];
> struct mgmt_ev_device_found *ev = (void *)buf;
> size_t ev_size;
> + struct hci_conn *hcon;
>
> /* Don't send events for a non-kernel initiated discovery. With
>* LE one exception is if we have pend_le_reports > 0 in which
> @@ -7521,6 +7562,18 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
> *bdaddr, u8 link_type,
> ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
> ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>
> + /* Search for Slave Connection Interval AD */
> + hcon = hci_conn_hash_lookup_le(hdev, bdaddr, addr_type);
> + if (hcon) {
> + u16 min_conn_int, max_conn_int;
> +
> + if (has_eir_slave_conn_int(ev->eir, ev->eir_len,
> +_conn_int, _conn_int)) {
> + hcon->le_conn_min_interval = min_conn_int;
> + hcon->le_conn_max_interval = max_conn_int;
> + }
> + }
> +
It's been some time that I looked at this code, so I could be missing
something, but I got the feeling that this part would make more sense if
it was at process_adv_report(), there's even the check for a pending
connection, so no need to redo that here.
Apart from this, the series looks good.
> mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> }
>
> --
> 2.12.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
Re: [PATCH] x86: Avoid undefined behavior in macro expansion
On Fri, Mar 18, 2016 at 11:15 PM, Al Viro <v...@zeniv.linux.org.uk> wrote: > On Wed, Mar 16, 2016 at 11:48:49PM -0300, Vinicius Tinti wrote: >> C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined >> behavior when one of the result operands is not a valid preprocessing >> token. >> >> Therefore the macro expansion may depend on compiler implementation >> which may or no preserve the leading white space. >> >> Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). >> Changing favors concise usage. > > Huh? > >> -#define XMM(i) CONCAT(%xmm, i) >> +#define XMM(i) CONCAT(%xmm,i) > > What are you talking about? Undefined behaviour is when the result of > concatenation of adjacent tokens is not a valid preprocessor token. > It says nothing about the either argument being a single token. Please check the example below otherwise it will be hard to explain. The problem is that _i_ can be a macro to be expanded too. And it can be a parameter for a _paste_ operator. // tricky code #define CONCAT(a,b)a##b #defineXMM(i)CONCAT(%xmm, i) .macro foo n x = XMM(\n) .endm _%xmm_ is not a problem but _i_ is. > In this case after the substitution of e.g. XMM(42) we get 3 tokens: > Punctuator[%] Identifier[xmm] Pp-number[42] > with ## instructing us to replace the last two with preprocessor token that > would be represented as concatenation of their representations. Which is > to say, concatenation of xmm and 42, i.e. xmm42. Which *is* a > representation of a valid preprocessor token - namely, Identifier[xmm42]. Agree. But it is not this case. I will add the code above at commit and describe it. It will be easy to explain what I am trying to solve. > No undefined behaviour at all. And yes, you get two preprocessor tokens > in the expansion - % and xmm42. Preprocessor works in terms of tokens, > not strings... Understood. > If you know of any compiler where these two variants would produce different > expansions of XMM(), please report it to maintainers of > the compiler in question; it's a bug, plain and simple. And no, there's > no undefined behaviour in that. I reported a bug and discussed over it and I too believe that the tricky code that I have just sent triggers an undefined behavior. What do you think? -- Simplicity is the ultimate sophistication
Re: [PATCH] x86: Avoid undefined behavior in macro expansion
On Fri, Mar 18, 2016 at 11:15 PM, Al Viro wrote: > On Wed, Mar 16, 2016 at 11:48:49PM -0300, Vinicius Tinti wrote: >> C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined >> behavior when one of the result operands is not a valid preprocessing >> token. >> >> Therefore the macro expansion may depend on compiler implementation >> which may or no preserve the leading white space. >> >> Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). >> Changing favors concise usage. > > Huh? > >> -#define XMM(i) CONCAT(%xmm, i) >> +#define XMM(i) CONCAT(%xmm,i) > > What are you talking about? Undefined behaviour is when the result of > concatenation of adjacent tokens is not a valid preprocessor token. > It says nothing about the either argument being a single token. Please check the example below otherwise it will be hard to explain. The problem is that _i_ can be a macro to be expanded too. And it can be a parameter for a _paste_ operator. // tricky code #define CONCAT(a,b)a##b #defineXMM(i)CONCAT(%xmm, i) .macro foo n x = XMM(\n) .endm _%xmm_ is not a problem but _i_ is. > In this case after the substitution of e.g. XMM(42) we get 3 tokens: > Punctuator[%] Identifier[xmm] Pp-number[42] > with ## instructing us to replace the last two with preprocessor token that > would be represented as concatenation of their representations. Which is > to say, concatenation of xmm and 42, i.e. xmm42. Which *is* a > representation of a valid preprocessor token - namely, Identifier[xmm42]. Agree. But it is not this case. I will add the code above at commit and describe it. It will be easy to explain what I am trying to solve. > No undefined behaviour at all. And yes, you get two preprocessor tokens > in the expansion - % and xmm42. Preprocessor works in terms of tokens, > not strings... Understood. > If you know of any compiler where these two variants would produce different > expansions of XMM(), please report it to maintainers of > the compiler in question; it's a bug, plain and simple. And no, there's > no undefined behaviour in that. I reported a bug and discussed over it and I too believe that the tricky code that I have just sent triggers an undefined behavior. What do you think? -- Simplicity is the ultimate sophistication
[PATCH] x86: Avoid undefined behavior in macro expansion
C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined behavior when one of the result operands is not a valid preprocessing token. Therefore the macro expansion may depend on compiler implementation which may or no preserve the leading white space. Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). Changing favors concise usage. Signed-off-by: Vinicius Tinti <viniciusti...@gmail.com> Acked-by: Behan Webster <beh...@converseincode.com> --- arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S index a916c4a..7a71553 100644 --- a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S +++ b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S @@ -93,7 +93,7 @@ #define tmp%r10 #defineDDQ(i) CONCAT(ddq_add_,i) -#defineXMM(i) CONCAT(%xmm, i) +#defineXMM(i) CONCAT(%xmm,i) #defineDDQ_DATA0 #defineXDATA 1 #define KEY_1281 -- 2.7.3
[PATCH] x86: Avoid undefined behavior in macro expansion
C11 standard (at 6.10.3.3) says that ## operator (paste) has undefined behavior when one of the result operands is not a valid preprocessing token. Therefore the macro expansion may depend on compiler implementation which may or no preserve the leading white space. Moreover other places in kernel use CONCAT(a,b) instead of CONCAT(a, b). Changing favors concise usage. Signed-off-by: Vinicius Tinti Acked-by: Behan Webster --- arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S index a916c4a..7a71553 100644 --- a/arch/x86/crypto/aes_ctrby8_avx-x86_64.S +++ b/arch/x86/crypto/aes_ctrby8_avx-x86_64.S @@ -93,7 +93,7 @@ #define tmp%r10 #defineDDQ(i) CONCAT(ddq_add_,i) -#defineXMM(i) CONCAT(%xmm, i) +#defineXMM(i) CONCAT(%xmm,i) #defineDDQ_DATA0 #defineXDATA 1 #define KEY_1281 -- 2.7.3
Re: Bluetooth related possible system hard lockups
Hi Jamin, Jamin Collins writes: > I've recently started experiencing what appear to be hard lockups on > what has otherwise been a very solid system (Lenovo W530 Thinkpad). > I'm running a fully updated Arch Linux install on it, and have been > for a few months (since roughly April of this year). Just before > experiencing these lockups I started using a bluetooth mouse. > > When the apparent lockup occurs: > - the laptop screen seems to be off (no display) > - the CPU fan is running at a moderate speed (not the slowest, and not > the fastest) > - no apparent disk activity > - nothing in journalctl (when I hard reset I find only a gap, last > entry hours before) > - unresponsive to network requests (ping, etc) > > The only way I've found to recover the system thus far is a hard reset > (hold power for 4+ seconds). > Without more information (the backtrace of the crash, from the kernel logs or even a picture of the screen showing the crash dump), it is hard to be sure. I can only guess. This looks like this issue (for one colleague on a very similar setup as yours, it was the case): http://thread.gmane.org/gmane.linux.kernel.input/45023 I just checked, that patch is in the stable queue, and for little it missed the 4.1.6 deadline, but it should appear on 4.1.7 (if it is released) or 4.2 (for sure). You can also apply that patch locally and see if solves your problem, or you can give the input subsystem kernel tree a try: https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git Cheers, -- Vinicius -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Bluetooth related possible system hard lockups
Hi Jamin, Jamin Collins jamin.coll...@gmail.com writes: I've recently started experiencing what appear to be hard lockups on what has otherwise been a very solid system (Lenovo W530 Thinkpad). I'm running a fully updated Arch Linux install on it, and have been for a few months (since roughly April of this year). Just before experiencing these lockups I started using a bluetooth mouse. When the apparent lockup occurs: - the laptop screen seems to be off (no display) - the CPU fan is running at a moderate speed (not the slowest, and not the fastest) - no apparent disk activity - nothing in journalctl (when I hard reset I find only a gap, last entry hours before) - unresponsive to network requests (ping, etc) The only way I've found to recover the system thus far is a hard reset (hold power for 4+ seconds). Without more information (the backtrace of the crash, from the kernel logs or even a picture of the screen showing the crash dump), it is hard to be sure. I can only guess. This looks like this issue (for one colleague on a very similar setup as yours, it was the case): http://thread.gmane.org/gmane.linux.kernel.input/45023 I just checked, that patch is in the stable queue, and for little it missed the 4.1.6 deadline, but it should appear on 4.1.7 (if it is released) or 4.2 (for sure). You can also apply that patch locally and see if solves your problem, or you can give the input subsystem kernel tree a try: https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git Cheers, -- Vinicius -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: annotate psci invoke functions as notrace
On Tue, Feb 24, 2015 at 3:11 PM, Mark Rutland wrote:
> On Tue, Feb 24, 2015 at 05:59:50PM +, Richard W.M. Jones wrote:
>> On Wed, Feb 18, 2015 at 12:26:38PM -0500, Kyle McMartin wrote:
>> > Using GCC 5 to build the kernel with ftrace enabled, we encounter the
>> > following error as a result of the mcount prologue changing the expected
>> > register use of the function parameters,
>> >
>> > /tmp/cc8Kpn7A.s: Assembler messages:
>> > /tmp/cc8Kpn7A.s:41: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:42: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:43: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:101: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:102: Error: .err encountered
>> > /tmp/cc8Kpn7A.s:103: Error: .err encountered
>> > scripts/Makefile.build:257: recipe for target 'arch/arm64/kernel/psci.o'
>> > failed
>> >
>> > Fix this by annotating the function as notrace, to suppress the
>> > generation of profiling prologues and epilogues on the function.
>> >
>> > Signed-off-by: Kyle McMartin
>> >
>> > --- a/arch/arm64/kernel/psci.c
>> > +++ b/arch/arm64/kernel/psci.c
>> > @@ -113,7 +113,7 @@ static void psci_power_state_unpack(u32 power_state,
>> > * The following two functions are invoked via the invoke_psci_fn pointer
>> > * and will not be inlined, allowing us to piggyback on the AAPCS.
>> > */
>> > -static noinline int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64
>> > arg1,
>> > +static noinline notrace int __invoke_psci_fn_hvc(u64 function_id, u64
>> > arg0, u64 arg1,
>> > u64 arg2)
>> > {
>> > asm volatile(
>> > @@ -128,7 +128,7 @@ static noinline int __invoke_psci_fn_hvc(u64
>> > function_id, u64 arg0, u64 arg1,
>> > return function_id;
>> > }
>> >
>> > -static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64
>> > arg1,
>> > +static noinline notrace int __invoke_psci_fn_smc(u64 function_id, u64
>> > arg0, u64 arg1,
>> > u64 arg2)
>> > {
>> > asm volatile(
>>
>> I need this patch in order to compile the upstream kernel on aarch64
>> using gcc 5. Can it not be added temporarily while the longer term
>> fix, whatever that is, is worked out?
>
> As I mentioned in my reply, Will was waiting for -rc1 to post our
> patches (which move this out to asm for arm and arm64). He's out of the
> office today, but I expect they will be posted tomorrow (and hopefully
> queued shortly thereafter).
>
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi,
I notice that the mainline kernel moved these psci calls to a separate
file but I
was wondering how can it guarantee that the function register
placement will hold?
If you build the kernel with -O0 some function register allocation changes as
opposed to -O2 or if you use another compiler such as Clang. In LLVMLinux we
solved this by using one of Andy's solution which is to use register placement:
register u32 function_id_r0 asm ("r0") = function_id;
register u32 arg0_r1 asm ("r1") = arg0;
register u32 arg1_r2 asm ("r2") = arg1;
register u32 arg2_r3 asm ("r3") = arg2;
Sorry by the late reply.
Regards,
Tinti
--
Simplicity is the ultimate sophistication
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: annotate psci invoke functions as notrace
On Tue, Feb 24, 2015 at 3:11 PM, Mark Rutland mark.rutl...@arm.com wrote:
On Tue, Feb 24, 2015 at 05:59:50PM +, Richard W.M. Jones wrote:
On Wed, Feb 18, 2015 at 12:26:38PM -0500, Kyle McMartin wrote:
Using GCC 5 to build the kernel with ftrace enabled, we encounter the
following error as a result of the mcount prologue changing the expected
register use of the function parameters,
/tmp/cc8Kpn7A.s: Assembler messages:
/tmp/cc8Kpn7A.s:41: Error: .err encountered
/tmp/cc8Kpn7A.s:42: Error: .err encountered
/tmp/cc8Kpn7A.s:43: Error: .err encountered
/tmp/cc8Kpn7A.s:101: Error: .err encountered
/tmp/cc8Kpn7A.s:102: Error: .err encountered
/tmp/cc8Kpn7A.s:103: Error: .err encountered
scripts/Makefile.build:257: recipe for target 'arch/arm64/kernel/psci.o'
failed
Fix this by annotating the function as notrace, to suppress the
generation of profiling prologues and epilogues on the function.
Signed-off-by: Kyle McMartin k...@redhat.com
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -113,7 +113,7 @@ static void psci_power_state_unpack(u32 power_state,
* The following two functions are invoked via the invoke_psci_fn pointer
* and will not be inlined, allowing us to piggyback on the AAPCS.
*/
-static noinline int __invoke_psci_fn_hvc(u64 function_id, u64 arg0, u64
arg1,
+static noinline notrace int __invoke_psci_fn_hvc(u64 function_id, u64
arg0, u64 arg1,
u64 arg2)
{
asm volatile(
@@ -128,7 +128,7 @@ static noinline int __invoke_psci_fn_hvc(u64
function_id, u64 arg0, u64 arg1,
return function_id;
}
-static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64
arg1,
+static noinline notrace int __invoke_psci_fn_smc(u64 function_id, u64
arg0, u64 arg1,
u64 arg2)
{
asm volatile(
I need this patch in order to compile the upstream kernel on aarch64
using gcc 5. Can it not be added temporarily while the longer term
fix, whatever that is, is worked out?
As I mentioned in my reply, Will was waiting for -rc1 to post our
patches (which move this out to asm for arm and arm64). He's out of the
office today, but I expect they will be posted tomorrow (and hopefully
queued shortly thereafter).
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Hi,
I notice that the mainline kernel moved these psci calls to a separate
file but I
was wondering how can it guarantee that the function register
placement will hold?
If you build the kernel with -O0 some function register allocation changes as
opposed to -O2 or if you use another compiler such as Clang. In LLVMLinux we
solved this by using one of Andy's solution which is to use register placement:
register u32 function_id_r0 asm (r0) = function_id;
register u32 arg0_r1 asm (r1) = arg0;
register u32 arg1_r2 asm (r2) = arg1;
register u32 arg2_r3 asm (r3) = arg2;
Sorry by the late reply.
Regards,
Tinti
--
Simplicity is the ultimate sophistication
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arm: remove extra semicolon in if statement
Remove extra semicolon in perf_event.c if statement.
Signed-off-by: Vinicius Tinti
---
arch/arm/kernel/perf_event.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 31e0eb3..a892067 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -400,7 +400,7 @@ __hw_perf_event_init(struct perf_event *event)
}
if (event->group_leader != event) {
- if (validate_group(event) != 0);
+ if (validate_group(event) != 0)
return -EINVAL;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] arm: remove extra semicolon in if statement
Remove extra semicolon in perf_event.c if statement.
Signed-off-by: Vinicius Tinti viniciusti...@gmail.com
---
arch/arm/kernel/perf_event.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 31e0eb3..a892067 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -400,7 +400,7 @@ __hw_perf_event_init(struct perf_event *event)
}
if (event-group_leader != event) {
- if (validate_group(event) != 0);
+ if (validate_group(event) != 0)
return -EINVAL;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Kernel doesn't free Cached Memory
Em (15:49:49), Alan Cox escreveu: >On Gwe, 2005-07-22 at 08:27 -0300, Vinicius wrote: >> Hi all! >> >> I have a server with 2 Pentium 4 HT processors and 32 GB of RAM, this >> server runs lots of applications that consume lots of memory to. When I >stop >> this applications, the kernel doesn't free memory (the memory still in >use) > >See any FAQ on the Linux memory management - memory is reclaimed when >needed not when nobody is using it. That makes things more efficient. > >> and the server cache lots of memory (~27GB). When I start this >applications, >> the kernel sends "Out of Memory" messages and kill some random >> applications. > >Some RHEL3 kernels had a problem with very large memory sizes and 2.4. >That should not be the case in the current RHEL3 kernels. 2.6 handles >very large systems a lot lot better, and of course the fact real >computers now have 64bit processors has also rather improved life. > >Alan > >-- Thanks Alan, I also read on the Linux-Kernel that the problem may be related to an exhaustion of your kernels address space, I read that the hugemem-kernel might be the solution to this case since it has 4GB for the kernel memory plus 4GB for user process. How can I define if my kernel memory is beeing exhausted? Does this exhaustion of kernel memory can cause Out Of memory errors ?
Re: Re: Kernel doesn't free Cached Memory
On Fri, 2005-07-22 at 08:27 -0300, Vinicius wrote:
[...]
>>I have a server with 2 Pentium 4 HT processors and 32 GB of >>RAM,
this
>> server runs lots of applications that consume lots of memory to. >>When I
>>stop
>> this applications, the kernel doesn't free memory (the memory >>still in
>>use)
>> and the server cache lots of memory (~27GB). When I start this
>>applications,
>> the kernel sends "Out of Memory" messages and kill some random
>> applications.
>>
>>Anyone know how can I reduce the kernel cached memory on RHEL >>3
(kernel
>> 2.4.21-32.ELsmp - Trial version)? There is a way to reduce the >>kernel
>>cached
>> memory utilization?
>Probably RedHat's support can answer this for RHEL 3.
>
> Bernd
>--
> Firmix Software GmbH http://www.firmix.at/
>mobil: +43 664 4416156 fax: +43 1 7890849-55
> Embedded Linux Development and Services
Bernd,
The server runs RHEL Trial Version, without support... for tests purpose.
When I compile and run the following tester program:
#include
#include
#include
int main (void) {
int n = 0;
char *p;
while (1) {
if ((p = malloc(1<<20)) == NULL) {
printf("malloc failure after %d MiB\n", n);
return 0;
}
memset (p, 0, (1<<20));
printf ("got %d MiB\n", ++n);
}
}
The server alocates lots of free memory (including swap) to the tester
program and when its finish, lots of cached memory are freed.
Have someone an idea why it's happens? Or how can I force the kernel to
frees cached memory?
Thanks again (sorry my bad eglish again!)
Vinicius.
Protolink Consultoria.
Kernel doesn't free Cached Memory
Hi all! I have a server with 2 Pentium 4 HT processors and 32 GB of RAM, this server runs lots of applications that consume lots of memory to. When I stop this applications, the kernel doesn't free memory (the memory still in use) and the server cache lots of memory (~27GB). When I start this applications, the kernel sends "Out of Memory" messages and kill some random applications. Anyone know how can I reduce the kernel cached memory on RHEL 3 (kernel 2.4.21-32.ELsmp - Trial version)? There is a way to reduce the kernel cached memory utilization? Thanks in advance (sorry my bad english). Vinicius. Protolink Consultoria.
Kernel doesn't free Cached Memory
Hi all! I have a server with 2 Pentium 4 HT processors and 32 GB of RAM, this server runs lots of applications that consume lots of memory to. When I stop this applications, the kernel doesn't free memory (the memory still in use) and the server cache lots of memory (~27GB). When I start this applications, the kernel sends Out of Memory messages and kill some random applications. Anyone know how can I reduce the kernel cached memory on RHEL 3 (kernel 2.4.21-32.ELsmp - Trial version)? There is a way to reduce the kernel cached memory utilization? Thanks in advance (sorry my bad english). Vinicius. Protolink Consultoria.
Re: Re: Kernel doesn't free Cached Memory
On Fri, 2005-07-22 at 08:27 -0300, Vinicius wrote:
[...]
I have a server with 2 Pentium 4 HT processors and 32 GB of RAM,
this
server runs lots of applications that consume lots of memory to. When I
stop
this applications, the kernel doesn't free memory (the memory still in
use)
and the server cache lots of memory (~27GB). When I start this
applications,
the kernel sends Out of Memory messages and kill some random
applications.
Anyone know how can I reduce the kernel cached memory on RHEL 3
(kernel
2.4.21-32.ELsmp - Trial version)? There is a way to reduce the kernel
cached
memory utilization?
Probably RedHat's support can answer this for RHEL 3.
Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services
Bernd,
The server runs RHEL Trial Version, without support... for tests purpose.
When I compile and run the following tester program:
#include stdio.h
#include string.h
#include stdlib.h
int main (void) {
int n = 0;
char *p;
while (1) {
if ((p = malloc(120)) == NULL) {
printf(malloc failure after %d MiB\n, n);
return 0;
}
memset (p, 0, (120));
printf (got %d MiB\n, ++n);
}
}
The server alocates lots of free memory (including swap) to the tester
program and when its finish, lots of cached memory are freed.
Have someone an idea why it's happens? Or how can I force the kernel to
frees cached memory?
Thanks again (sorry my bad eglish again!)
Vinicius.
Protolink Consultoria.
Re: Kernel doesn't free Cached Memory
Em (15:49:49), Alan Cox escreveu: On Gwe, 2005-07-22 at 08:27 -0300, Vinicius wrote: Hi all! I have a server with 2 Pentium 4 HT processors and 32 GB of RAM, this server runs lots of applications that consume lots of memory to. When I stop this applications, the kernel doesn't free memory (the memory still in use) See any FAQ on the Linux memory management - memory is reclaimed when needed not when nobody is using it. That makes things more efficient. and the server cache lots of memory (~27GB). When I start this applications, the kernel sends Out of Memory messages and kill some random applications. Some RHEL3 kernels had a problem with very large memory sizes and 2.4. That should not be the case in the current RHEL3 kernels. 2.6 handles very large systems a lot lot better, and of course the fact real computers now have 64bit processors has also rather improved life. Alan -- Thanks Alan, I also read on the Linux-Kernel that the problem may be related to an exhaustion of your kernels address space, I read that the hugemem-kernel might be the solution to this case since it has 4GB for the kernel memory plus 4GB for user process. How can I define if my kernel memory is beeing exhausted? Does this exhaustion of kernel memory can cause Out Of memory errors ?
Gravador de telefone
Não seja o último saber. Chegou o gravador de conversas telefônicas. Trata-se de um mini gravador que pode ser instalado em qualquer ponto do fio da sua linha telefônica (inclusive extenções), e basta o seu telefone sair do gancho, seja para receber ou efetuar uma chamada, que a gravação é acionada automaticamente. E quando o telefone voltar para o gancho a gravação é finalizada. A gravação só é acionada quando o telefone está sendo utilizado. Depois é só voltar a fita e ouvir tudo o que foi falado. Essa técnologia está ao seu alcance por apenas R$ 185,00. Despachamos para todo o Brasil. Contato com Sérgio nos fones: 51-471.3583(pela manhã) * 51-8424.1442 (24 hs ) ou [EMAIL PROTECTED] Obs: Instalar esse aparelho em linhas alheias é contra a lei. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Gravador de telefone
Não seja o último saber. Chegou o gravador de conversas telefônicas. Trata-se de um mini gravador que pode ser instalado em qualquer ponto do fio da sua linha telefônica (inclusive extenções), e basta o seu telefone sair do gancho, seja para receber ou efetuar uma chamada, que a gravação é acionada automaticamente. E quando o telefone voltar para o gancho a gravação é finalizada. A gravação só é acionada quando o telefone está sendo utilizado. Depois é só voltar a fita e ouvir tudo o que foi falado. Essa técnologia está ao seu alcance por apenas R$ 185,00. Despachamos para todo o Brasil. Contato com Sérgio nos fones: 51-471.3583(pela manhã) * 51-8424.1442 (24 hs ) ou [EMAIL PROTECTED] Obs: Instalar esse aparelho em linhas alheias é contra a lei. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
