Commit 655c6b97 authored by Linus Torvalds's avatar Linus Torvalds

Merge branch 'perf-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

Pull perf fixes from Thomas Gleixner:
 "A bunch of perf tooling fixes:

   - Make the Intel PT SQL viewer more robust

   - Make the Intel PT debug log more useful

   - Support weak groups in perf record so it's behaving the same way as
     perf stat

   - Display the LBR stats in callchain entries properly in perf top

   - Handle different PMu names with common prefix properlin in pert
     stat

   - Start syscall augmenting in perf trace. Preparation for
     architecture independent eBPF instrumentation of syscalls.

   - Fix build breakage in JVMTI perf lib

   - Fix arm64 tools build failure wrt smp_load_{acquire,release}"

* 'perf-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip:
  perf tools: Do not zero sample_id_all for group members
  perf tools: Fix undefined symbol scnprintf in libperf-jvmti.so
  perf beauty: Use SRCARCH, ARCH=x86_64 must map to "x86" to find the headers
  perf intel-pt: Add MTC and CYC timestamps to debug log
  perf intel-pt: Add more event information to debug log
  perf scripts python: exported-sql-viewer.py: Fix table find when table re-ordered
  perf scripts python: exported-sql-viewer.py: Add help window
  perf scripts python: exported-sql-viewer.py: Add Selected branches report
  perf scripts python: exported-sql-viewer.py: Fall back to /usr/local/lib/libxed.so
  perf top: Display the LBR stats in callchain entry
  perf stat: Handle different PMU names with common prefix
  perf record: Support weak groups
  perf evlist: Move perf_evsel__reset_weak_group into evlist
  perf augmented_syscalls: Start collecting pathnames in the BPF program
  perf trace: Fix setting of augmented payload when using eBPF + raw_syscalls
  perf trace: When augmenting raw_syscalls plug raw_syscalls:sys_exit too
  perf examples bpf: Start augmenting raw_syscalls:sys_{start,exit}
  tools headers barrier: Fix arm64 tools build failure wrt smp_load_{acquire,release}
parents 08b52786 45fd8080
......@@ -14,74 +14,75 @@
#define wmb() asm volatile("dmb ishst" ::: "memory")
#define rmb() asm volatile("dmb ishld" ::: "memory")
#define smp_store_release(p, v) \
do { \
union { typeof(*p) __val; char __c[1]; } __u = \
{ .__val = (__force typeof(*p)) (v) }; \
\
switch (sizeof(*p)) { \
case 1: \
asm volatile ("stlrb %w1, %0" \
: "=Q" (*p) \
: "r" (*(__u8 *)__u.__c) \
: "memory"); \
break; \
case 2: \
asm volatile ("stlrh %w1, %0" \
: "=Q" (*p) \
: "r" (*(__u16 *)__u.__c) \
: "memory"); \
break; \
case 4: \
asm volatile ("stlr %w1, %0" \
: "=Q" (*p) \
: "r" (*(__u32 *)__u.__c) \
: "memory"); \
break; \
case 8: \
asm volatile ("stlr %1, %0" \
: "=Q" (*p) \
: "r" (*(__u64 *)__u.__c) \
: "memory"); \
break; \
default: \
/* Only to shut up gcc ... */ \
mb(); \
break; \
} \
#define smp_store_release(p, v) \
do { \
union { typeof(*p) __val; char __c[1]; } __u = \
{ .__val = (v) }; \
\
switch (sizeof(*p)) { \
case 1: \
asm volatile ("stlrb %w1, %0" \
: "=Q" (*p) \
: "r" (*(__u8_alias_t *)__u.__c) \
: "memory"); \
break; \
case 2: \
asm volatile ("stlrh %w1, %0" \
: "=Q" (*p) \
: "r" (*(__u16_alias_t *)__u.__c) \
: "memory"); \
break; \
case 4: \
asm volatile ("stlr %w1, %0" \
: "=Q" (*p) \
: "r" (*(__u32_alias_t *)__u.__c) \
: "memory"); \
break; \
case 8: \
asm volatile ("stlr %1, %0" \
: "=Q" (*p) \
: "r" (*(__u64_alias_t *)__u.__c) \
: "memory"); \
break; \
default: \
/* Only to shut up gcc ... */ \
mb(); \
break; \
} \
} while (0)
#define smp_load_acquire(p) \
({ \
union { typeof(*p) __val; char __c[1]; } __u; \
\
switch (sizeof(*p)) { \
case 1: \
asm volatile ("ldarb %w0, %1" \
: "=r" (*(__u8 *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
case 2: \
asm volatile ("ldarh %w0, %1" \
: "=r" (*(__u16 *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
case 4: \
asm volatile ("ldar %w0, %1" \
: "=r" (*(__u32 *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
case 8: \
asm volatile ("ldar %0, %1" \
: "=r" (*(__u64 *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
default: \
/* Only to shut up gcc ... */ \
mb(); \
break; \
} \
__u.__val; \
#define smp_load_acquire(p) \
({ \
union { typeof(*p) __val; char __c[1]; } __u = \
{ .__c = { 0 } }; \
\
switch (sizeof(*p)) { \
case 1: \
asm volatile ("ldarb %w0, %1" \
: "=r" (*(__u8_alias_t *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
case 2: \
asm volatile ("ldarh %w0, %1" \
: "=r" (*(__u16_alias_t *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
case 4: \
asm volatile ("ldar %w0, %1" \
: "=r" (*(__u32_alias_t *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
case 8: \
asm volatile ("ldar %0, %1" \
: "=r" (*(__u64_alias_t *)__u.__c) \
: "Q" (*p) : "memory"); \
break; \
default: \
/* Only to shut up gcc ... */ \
mb(); \
break; \
} \
__u.__val; \
})
#endif /* _TOOLS_LINUX_ASM_AARCH64_BARRIER_H */
......@@ -55,7 +55,6 @@ counted. The following modifiers exist:
S - read sample value (PERF_SAMPLE_READ)
D - pin the event to the PMU
W - group is weak and will fallback to non-group if not schedulable,
only supported in 'perf stat' for now.
The 'p' modifier can be used for specifying how precise the instruction
address should be. The 'p' modifier can be specified multiple times:
......
......@@ -387,7 +387,7 @@ SHELL = $(SHELL_PATH)
linux_uapi_dir := $(srctree)/tools/include/uapi/linux
asm_generic_uapi_dir := $(srctree)/tools/include/uapi/asm-generic
arch_asm_uapi_dir := $(srctree)/tools/arch/$(ARCH)/include/uapi/asm/
arch_asm_uapi_dir := $(srctree)/tools/arch/$(SRCARCH)/include/uapi/asm/
beauty_outdir := $(OUTPUT)trace/beauty/generated
beauty_ioctl_outdir := $(beauty_outdir)/ioctl
......
......@@ -391,7 +391,12 @@ static int record__open(struct record *rec)
ui__warning("%s\n", msg);
goto try_again;
}
if ((errno == EINVAL || errno == EBADF) &&
pos->leader != pos &&
pos->weak_group) {
pos = perf_evlist__reset_weak_group(evlist, pos);
goto try_again;
}
rc = -errno;
perf_evsel__open_strerror(pos, &opts->target,
errno, msg, sizeof(msg));
......
......@@ -383,32 +383,6 @@ static bool perf_evsel__should_store_id(struct perf_evsel *counter)
return STAT_RECORD || counter->attr.read_format & PERF_FORMAT_ID;
}
static struct perf_evsel *perf_evsel__reset_weak_group(struct perf_evsel *evsel)
{
struct perf_evsel *c2, *leader;
bool is_open = true;
leader = evsel->leader;
pr_debug("Weak group for %s/%d failed\n",
leader->name, leader->nr_members);
/*
* for_each_group_member doesn't work here because it doesn't
* include the first entry.
*/
evlist__for_each_entry(evsel_list, c2) {
if (c2 == evsel)
is_open = false;
if (c2->leader == leader) {
if (is_open)
perf_evsel__close(c2);
c2->leader = c2;
c2->nr_members = 0;
}
}
return leader;
}
static bool is_target_alive(struct target *_target,
struct thread_map *threads)
{
......@@ -477,7 +451,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
if ((errno == EINVAL || errno == EBADF) &&
counter->leader != counter &&
counter->weak_group) {
counter = perf_evsel__reset_weak_group(counter);
counter = perf_evlist__reset_weak_group(evsel_list, counter);
goto try_again;
}
......
......@@ -1429,6 +1429,9 @@ int cmd_top(int argc, const char **argv)
}
}
if (opts->branch_stack && callchain_param.enabled)
symbol_conf.show_branchflag_count = true;
sort__mode = SORT_MODE__TOP;
/* display thread wants entries to be collapsed in a different tree */
perf_hpp_list.need_collapse = 1;
......
......@@ -108,6 +108,7 @@ struct trace {
} stats;
unsigned int max_stack;
unsigned int min_stack;
bool raw_augmented_syscalls;
bool not_ev_qualifier;
bool live;
bool full_time;
......@@ -1724,13 +1725,28 @@ static int trace__fprintf_sample(struct trace *trace, struct perf_evsel *evsel,
return printed;
}
static void *syscall__augmented_args(struct syscall *sc, struct perf_sample *sample, int *augmented_args_size)
static void *syscall__augmented_args(struct syscall *sc, struct perf_sample *sample, int *augmented_args_size, bool raw_augmented)
{
void *augmented_args = NULL;
/*
* For now with BPF raw_augmented we hook into raw_syscalls:sys_enter
* and there we get all 6 syscall args plus the tracepoint common
* fields (sizeof(long)) and the syscall_nr (another long). So we check
* if that is the case and if so don't look after the sc->args_size,
* but always after the full raw_syscalls:sys_enter payload, which is
* fixed.
*
* We'll revisit this later to pass s->args_size to the BPF augmenter
* (now tools/perf/examples/bpf/augmented_raw_syscalls.c, so that it
* copies only what we need for each syscall, like what happens when we
* use syscalls:sys_enter_NAME, so that we reduce the kernel/userspace
* traffic to just what is needed for each syscall.
*/
int args_size = raw_augmented ? (8 * (int)sizeof(long)) : sc->args_size;
*augmented_args_size = sample->raw_size - sc->args_size;
*augmented_args_size = sample->raw_size - args_size;
if (*augmented_args_size > 0)
augmented_args = sample->raw_data + sc->args_size;
augmented_args = sample->raw_data + args_size;
return augmented_args;
}
......@@ -1780,7 +1796,7 @@ static int trace__sys_enter(struct trace *trace, struct perf_evsel *evsel,
* here and avoid using augmented syscalls when the evsel is the raw_syscalls one.
*/
if (evsel != trace->syscalls.events.sys_enter)
augmented_args = syscall__augmented_args(sc, sample, &augmented_args_size);
augmented_args = syscall__augmented_args(sc, sample, &augmented_args_size, trace->raw_augmented_syscalls);
ttrace->entry_time = sample->time;
msg = ttrace->entry_str;
printed += scnprintf(msg + printed, trace__entry_str_size - printed, "%s(", sc->name);
......@@ -1833,7 +1849,7 @@ static int trace__fprintf_sys_enter(struct trace *trace, struct perf_evsel *evse
goto out_put;
args = perf_evsel__sc_tp_ptr(evsel, args, sample);
augmented_args = syscall__augmented_args(sc, sample, &augmented_args_size);
augmented_args = syscall__augmented_args(sc, sample, &augmented_args_size, trace->raw_augmented_syscalls);
syscall__scnprintf_args(sc, msg, sizeof(msg), args, augmented_args, augmented_args_size, trace, thread);
fprintf(trace->output, "%s", msg);
err = 0;
......@@ -3501,7 +3517,15 @@ int cmd_trace(int argc, const char **argv)
evsel->handler = trace__sys_enter;
evlist__for_each_entry(trace.evlist, evsel) {
bool raw_syscalls_sys_exit = strcmp(perf_evsel__name(evsel), "raw_syscalls:sys_exit") == 0;
if (raw_syscalls_sys_exit) {
trace.raw_augmented_syscalls = true;
goto init_augmented_syscall_tp;
}
if (strstarts(perf_evsel__name(evsel), "syscalls:sys_exit_")) {
init_augmented_syscall_tp:
perf_evsel__init_augmented_syscall_tp(evsel);
perf_evsel__init_augmented_syscall_tp_ret(evsel);
evsel->handler = trace__sys_exit;
......
// SPDX-License-Identifier: GPL-2.0
/*
* Augment the raw_syscalls tracepoints with the contents of the pointer arguments.
*
* Test it with:
*
* perf trace -e tools/perf/examples/bpf/augmented_raw_syscalls.c cat /etc/passwd > /dev/null
*
* This exactly matches what is marshalled into the raw_syscall:sys_enter
* payload expected by the 'perf trace' beautifiers.
*
* For now it just uses the existing tracepoint augmentation code in 'perf
* trace', in the next csets we'll hook up these with the sys_enter/sys_exit
* code that will combine entry/exit in a strace like way.
*/
#include <stdio.h>
#include <linux/socket.h>
/* bpf-output associated map */
struct bpf_map SEC("maps") __augmented_syscalls__ = {
.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
.key_size = sizeof(int),
.value_size = sizeof(u32),
.max_entries = __NR_CPUS__,
};
struct syscall_enter_args {
unsigned long long common_tp_fields;
long syscall_nr;
unsigned long args[6];
};
struct syscall_exit_args {
unsigned long long common_tp_fields;
long syscall_nr;
long ret;
};
struct augmented_filename {
unsigned int size;
int reserved;
char value[256];
};
#define SYS_OPEN 2
#define SYS_OPENAT 257
SEC("raw_syscalls:sys_enter")
int sys_enter(struct syscall_enter_args *args)
{
struct {
struct syscall_enter_args args;
struct augmented_filename filename;
} augmented_args;
unsigned int len = sizeof(augmented_args);
const void *filename_arg = NULL;
probe_read(&augmented_args.args, sizeof(augmented_args.args), args);
/*
* Yonghong and Edward Cree sayz:
*
* https://www.spinics.net/lists/netdev/msg531645.html
*
* >> R0=inv(id=0) R1=inv2 R6=ctx(id=0,off=0,imm=0) R7=inv64 R10=fp0,call_-1
* >> 10: (bf) r1 = r6
* >> 11: (07) r1 += 16
* >> 12: (05) goto pc+2
* >> 15: (79) r3 = *(u64 *)(r1 +0)
* >> dereference of modified ctx ptr R1 off=16 disallowed
* > Aha, we at least got a different error message this time.
* > And indeed llvm has done that optimisation, rather than the more obvious
* > 11: r3 = *(u64 *)(r1 +16)
* > because it wants to have lots of reads share a single insn. You may be able
* > to defeat that optimisation by adding compiler barriers, idk. Maybe someone
* > with llvm knowledge can figure out how to stop it (ideally, llvm would know
* > when it's generating for bpf backend and not do that). -O0? ¯\_(ツ)_/¯
*
* The optimization mostly likes below:
*
* br1:
* ...
* r1 += 16
* goto merge
* br2:
* ...
* r1 += 20
* goto merge
* merge:
* *(u64 *)(r1 + 0)
*
* The compiler tries to merge common loads. There is no easy way to
* stop this compiler optimization without turning off a lot of other
* optimizations. The easiest way is to add barriers:
*
* __asm__ __volatile__("": : :"memory")
*
* after the ctx memory access to prevent their down stream merging.
*/
switch (augmented_args.args.syscall_nr) {
case SYS_OPEN: filename_arg = (const void *)args->args[0];
__asm__ __volatile__("": : :"memory");
break;
case SYS_OPENAT: filename_arg = (const void *)args->args[1];
break;
}
if (filename_arg != NULL) {
augmented_args.filename.reserved = 0;
augmented_args.filename.size = probe_read_str(&augmented_args.filename.value,
sizeof(augmented_args.filename.value),
filename_arg);
if (augmented_args.filename.size < sizeof(augmented_args.filename.value)) {
len -= sizeof(augmented_args.filename.value) - augmented_args.filename.size;
len &= sizeof(augmented_args.filename.value) - 1;
}
} else {
len = sizeof(augmented_args.args);
}
perf_event_output(args, &__augmented_syscalls__, BPF_F_CURRENT_CPU, &augmented_args, len);
return 0;
}
SEC("raw_syscalls:sys_exit")
int sys_exit(struct syscall_exit_args *args)
{
return 1; /* 0 as soon as we start copying data returned by the kernel, e.g. 'read' */
}
license(GPL);
......@@ -125,7 +125,7 @@ perf_get_timestamp(void)
}
static int
debug_cache_init(void)
create_jit_cache_dir(void)
{
char str[32];
char *base, *p;
......@@ -144,8 +144,13 @@ debug_cache_init(void)
strftime(str, sizeof(str), JIT_LANG"-jit-%Y%m%d", &tm);
snprintf(jit_path, PATH_MAX - 1, "%s/.debug/", base);
ret = snprintf(jit_path, PATH_MAX, "%s/.debug/", base);
if (ret >= PATH_MAX) {
warnx("jvmti: cannot generate jit cache dir because %s/.debug/"
" is too long, please check the cwd, JITDUMPDIR, and"
" HOME variables", base);
return -1;
}
ret = mkdir(jit_path, 0755);
if (ret == -1) {
if (errno != EEXIST) {
......@@ -154,20 +159,32 @@ debug_cache_init(void)
}
}
snprintf(jit_path, PATH_MAX - 1, "%s/.debug/jit", base);
ret = snprintf(jit_path, PATH_MAX, "%s/.debug/jit", base);
if (ret >= PATH_MAX) {
warnx("jvmti: cannot generate jit cache dir because"
" %s/.debug/jit is too long, please check the cwd,"
" JITDUMPDIR, and HOME variables", base);
return -1;
}
ret = mkdir(jit_path, 0755);
if (ret == -1) {
if (errno != EEXIST) {
warn("cannot create jit cache dir %s", jit_path);
warn("jvmti: cannot create jit cache dir %s", jit_path);
return -1;
}
}
snprintf(jit_path, PATH_MAX - 1, "%s/.debug/jit/%s.XXXXXXXX", base, str);
ret = snprintf(jit_path, PATH_MAX, "%s/.debug/jit/%s.XXXXXXXX", base, str);
if (ret >= PATH_MAX) {
warnx("jvmti: cannot generate jit cache dir because"
" %s/.debug/jit/%s.XXXXXXXX is too long, please check"
" the cwd, JITDUMPDIR, and HOME variables",
base, str);
return -1;
}
p = mkdtemp(jit_path);
if (p != jit_path) {
warn("cannot create jit cache dir %s", jit_path);
warn("jvmti: cannot create jit cache dir %s", jit_path);
return -1;
}
......@@ -228,7 +245,7 @@ void *jvmti_open(void)
{
char dump_path[PATH_MAX];
struct jitheader header;
int fd;
int fd, ret;
FILE *fp;
init_arch_timestamp();
......@@ -245,12 +262,22 @@ void *jvmti_open(void)
memset(&header, 0, sizeof(header));
debug_cache_init();
/*
* jitdump file dir
*/
if (create_jit_cache_dir() < 0)
return NULL;
/*
* jitdump file name
*/
scnprintf(dump_path, PATH_MAX, "%s/jit-%i.dump", jit_path, getpid());
ret = snprintf(dump_path, PATH_MAX, "%s/jit-%i.dump", jit_path, getpid());
if (ret >= PATH_MAX) {
warnx("jvmti: cannot generate jitdump file full path because"
" %s/jit-%i.dump is too long, please check the cwd,"
" JITDUMPDIR, and HOME variables", jit_path, getpid());
return NULL;
}
fd = open(dump_path, O_CREAT|O_TRUNC|O_RDWR, 0666);
if (fd == -1)
......
......@@ -37,4 +37,3 @@ sample_freq=0
sample_period=0
freq=0
write_backward=0
sample_id_all=0
......@@ -1810,3 +1810,30 @@ void perf_evlist__force_leader(struct perf_evlist *evlist)
leader->forced_leader = true;
}
}
struct perf_evsel *perf_evlist__reset_weak_group(struct perf_evlist *evsel_list,
struct perf_evsel *evsel)
{
struct perf_evsel *c2, *leader;
bool is_open = true;
leader = evsel->leader;
pr_debug("Weak group for %s/%d failed\n",
leader->name, leader->nr_members);
/*
* for_each_group_member doesn't work here because it doesn't
* include the first entry.
*/
evlist__for_each_entry(evsel_list, c2) {
if (c2 == evsel)
is_open = false;
if (c2->leader == leader) {
if (is_open)
perf_evsel__close(c2);
c2->leader = c2;
c2->nr_members = 0;
}
}
return leader;
}
......@@ -312,4 +312,7 @@ bool perf_evlist__exclude_kernel(struct perf_evlist *evlist);
void perf_evlist__force_leader(struct perf_evlist *evlist);
struct perf_evsel *perf_evlist__reset_weak_group(struct perf_evlist *evlist,
struct perf_evsel *evsel);
#endif /* __PERF_EVLIST_H */
......@@ -956,7 +956,6 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
attr->sample_freq = 0;
attr->sample_period = 0;
attr->write_backward = 0;
attr->sample_id_all = 0;
}
if (opts->no_samples)
......
......@@ -1474,6 +1474,8 @@ static void intel_pt_calc_mtc_timestamp(struct intel_pt_decoder *decoder)
decoder->have_calc_cyc_to_tsc = false;
intel_pt_calc_cyc_to_tsc(decoder, true);
}
intel_pt_log_to("Setting timestamp", decoder->timestamp);
}
static void intel_pt_calc_cbr(struct intel_pt_decoder *decoder)
......@@ -1514,6 +1516,8 @@ static void intel_pt_calc_cyc_timestamp(struct intel_pt_decoder *decoder)
decoder->timestamp = timestamp;
decoder->timestamp_insn_cnt = 0;
intel_pt_log_to("Setting timestamp", decoder->timestamp);
}
/* Walk PSB+ packets when already in sync. */
......
......@@ -31,6 +31,11 @@ static FILE *f;
static char log_name[MAX_LOG_NAME];
bool intel_pt_enable_logging;
void *intel_pt_log_fp(void)
{
return f;
}
void intel_pt_log_enable(void)
{
intel_pt_enable_logging = true;
......
......@@ -22,6 +22,7 @@
struct intel_pt_pkt;
void *intel_pt_log_fp(void);
void intel_pt_log_enable(void);
void intel_pt_log_disable(void);
void intel_pt_log_set_name(const char *name);
......
......@@ -206,6 +206,16 @@ static void intel_pt_dump_event(struct intel_pt *pt, unsigned char *buf,
intel_pt_dump(pt, buf, len);
}
static void intel_pt_log_event(union perf_event *event)
{
FILE *f = intel_pt_log_fp();
if (!intel_pt_enable_logging || !f)
return;
perf_event__fprintf(event, f);
}
static int intel_pt_do_fix_overlap(struct intel_pt *pt, struct auxtrace_buffer *a,
struct auxtrace_buffer *b)
{
......@@ -2010,9 +2020,9 @@ static int intel_pt_process_event(struct perf_session *session,
event->header.type == PERF_RECORD_SWITCH_CPU_WIDE)
err = intel_pt_context_switch(pt, event, sample);
intel_pt_log("event %s (%u): cpu %d time %"PRIu64" tsc %#"PRIx64"\n",
perf_event__name(event->header.type), event->header.type,
sample->cpu, sample->time, timestamp);
intel_pt_log("event %u: cpu %d time %"PRIu64" tsc %#"PRIx64" ",
event->header.type, sample->cpu, sample->time, timestamp);
intel_pt_log_event(event);
return err;
}
......
......@@ -773,7 +773,7 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
if (!is_arm_pmu_core(name)) {
pname = pe->pmu ? pe->pmu : "cpu";
if (strncmp(pname, name, strlen(pname)))
if (strcmp(pname, name))
continue;
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment