Re: [RFC PATCH 0/5] Add support for suppressing warning backtraces
On Tue, Mar 05, 2024 at 10:40:28AM -0800, Guenter Roeck wrote: > Some unit tests intentionally trigger warning backtraces by passing bad > parameters to kernel API functions. Such unit tests typically check the > return value from such calls, not the existence of the warning backtrace. > > Such intentionally generated warning backtraces are neither desirable > nor useful for a number of reasons. > - They can result in overlooked real problems. > - A warning that suddenly starts to show up in unit tests needs to be > investigated and has to be marked to be ignored, for example by > adjusting filter scripts. Such filters are ad-hoc because there is > no real standard format for warnings. On top of that, such filter > scripts would require constant maintenance. > > One option to address problem would be to add messages such as "expected > warning backtraces start / end here" to the kernel log. However, that > would again require filter scripts, it might result in missing real > problematic warning backtraces triggered while the test is running, and > the irrelevant backtrace(s) would still clog the kernel log. > > Solve the problem by providing a means to identify and suppress specific > warning backtraces while executing test code. Support suppressing multiple > backtraces while at the same time limiting changes to generic code to the > absolute minimum. Architecture specific changes are kept at minimum by > retaining function names only if both CONFIG_DEBUG_BUGVERBOSE and > CONFIG_KUNIT are enabled. > > The first patch of the series introduces the necessary infrastructure. > The second patch marks the warning message in drm_calc_scale() in the DRM > subsystem as intentional where warranted. This patch is intended to serve > as an example for the use of the functionality introduced with this series. > The last three patches in the series introduce the necessary architecture > specific changes for x86, arm64, and loongarch. > > This series is based on the RFC patch and subsequent discussion at > https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b08-ba81-d94f3b691c9a@moroto.mountain/ > and offers a more comprehensive solution of the problem discussed there. > > Checkpatch note: > Remaining checkpatch errors and warnings were deliberately ignored. > Some are triggered by matching coding style or by comments interpreted > as code, others by assembler macros which are disliked by checkpatch. > Suggestions for improvements are welcome. > > Some questions: > > - Is the general approach promising ? If not, are there other possible > solutions ? > - Function pointers are only added to the __bug_table section if both > CONFIG_KUNIT and CONFIG_DEBUG_BUGVERBOSE are enabled. This avoids image > size increases if CONFIG_KUNIT=n. Downside is slightly more complex > architecture specific assembler code. If function pointers were always > added to the __bug_table section, vmlinux image size would increase by > approximately 0.6-0.7%. Is the increased complexity in assembler code > worth the reduced image size ? I think so, but I would like to hear > other opinions. > - There are additional possibilities associated with storing the bug > function name in the __bug_table section. It could be independent of > KUNIT, it could be a configuration flag, and/or it could be used to > display the name of the offending function in BUG/WARN messages. > Is any of those of interest ? > I am ready to send a full version of this series with support for all affected architectures. I am undecided if I should send it now, based on v6.8, and send v2 after rebasing it to v6.9-rc1, or if I should just wait for v6.9-rc1. I understand that some maintainers dislike getting new patch series while the commit window is is open. On the ther side, I tested the series thoroughly on top of v6.8-rc7, and initial v6.9 release candidates may have their own problems. Given that, I tend to send the series now. Any thoughts ? Unless there is strong negative feedback, I'll likely do that in a day or two. Thanks, Guenter
Re: [RFC PATCH net-next v6 01/15] queue_api: define queue api
On 3/8/24 4:47 PM, David Wei wrote: > > I'm working to port bnxt over to using this API. What are your thoughts > on maybe pulling this out and use bnxt to drive it? > I would love to see a second nic implementation; this patch set and overall design is driven by GVE limitations.
Re: [PATCH 1/8] iommu: Introduce a replace API for device pasid
On 2024/1/16 01:19, Jason Gunthorpe wrote:
On Sun, Nov 26, 2023 at 10:34:21PM -0800, Yi Liu wrote:
+int iommu_replace_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct iommu_group *group = dev->iommu_group;
+ struct iommu_domain *old_domain;
+ int ret;
+
+ if (!domain)
+ return -EINVAL;
+
+ if (!group)
+ return -ENODEV;
+
+ mutex_lock(>mutex);
+ __iommu_remove_group_pasid(group, pasid);
It is not replace if you do remove first.
Replace must just call set_dev_pasid and nothing much else..
Seems uneasy to do it so far. The VT-d driver needs to get the old domain
first in order to do replacement. However, VT-d driver does not track the
attached domains of pasids. It gets domain of a pasid
by iommu_get_domain_for_dev_pasid(). Like intel_iommu_remove_dev_pasid)
in link [1]. While the iommu layer exchanges the domain in the
group->pasid_array before calling into VT-d driver. So when calling into
VT-d driver, the domain got by iommu_get_domain_for_dev_pasid() is already
the new domain. To solve it, we may need to let VT-d driver have its
own tracking on the domains. How about your thoughts? @Jason, @Kevin, @Baoplu?
[1]
https://github.com/torvalds/linux/blob/master/drivers/iommu/intel/iommu.c#L4621C19-L4621C20
--
Regards,
Yi Liu
Re: [PATCH v1 12/13] tools headers: Sync compiler.h headers
On Sun, Mar 10, 2024 at 3:06 AM Ian Rogers wrote: > > compiler_attributes.h - added from > include/linux/compiler_attributes.h, > guards were added to definitions to avoid redefinition of macros > in libc. Do you have an example for context? Thanks! Cheers, Miguel
Re: [PATCH v1 13/13] tools headers: Rename noinline to __noinline
On Sun, Mar 10, 2024 at 3:06 AM Ian Rogers wrote: > > [1] https://clang.llvm.org/docs/AttributeReference.html#noinline > Reported-by: Christopher Di Bella Out of curiosity, was this due to the `[[gnu::noinline]]` or similar in e.g. `src/string/memset_explicit.h`? > -#define noinline __attribute__((__noinline__)) > +#define __noinline __attribute__((__noinline__)) I guess it does not matter since I don't see it used in `tools/`, but should the one inside `__fix_address` be updated too? Or is the idea to keep the diff as minimal as possible? Cheers, Miguel
Re: [PATCH net-next v3 4/4] net: gro: move L3 flush checks to tcp_gro_receive
Richard Gobert wrote:
> {inet,ipv6}_gro_receive functions perform flush checks (ttl, flags,
> iph->id, ...) against all packets in a loop. These flush checks are
> relevant only to tcp flows, and as such they're used to determine whether
> the packets can be merged later in tcp_gro_receive.
>
> These checks are not relevant to UDP packets.
These are network protocol coalescing invariants. Why would they be
limited to certain transport protocols only?
> Furthermore, they need to be
> done only once in tcp_gro_receive and only against the found p skb, since
> they only affect flush and not same_flow.
>
> Levaraging the previous commit in the series, in which correct network
> header offsets are saved for both outer and inner network headers -
> allowing these checks to be done only once, in tcp_gro_receive. As a
> result, NAPI_GRO_CB(p)->flush is not used at all. In addition - flush_id
> checks are more declerative and contained in inet_gro_flush, thus removing
declarative
> the need for flush_id in napi_gro_cb.
>
> Signed-off-by: Richard Gobert
> ---
> +static int inet_gro_flush(const struct iphdr *iph, const struct iphdr *iph2,
> + struct sk_buff *p, u32 outer)
> +{
> + const u32 id = ntohl(*(__be32 *)>id);
> + const u32 id2 = ntohl(*(__be32 *)>id);
> + const int flush_id = ntohs(id >> 16) - ntohs(id2 >> 16);
> + const u16 count = NAPI_GRO_CB(p)->count;
> + const u32 df = id & IP_DF;
> + u32 is_atomic;
> + int flush;
> +
> + /* All fields must match except length and checksum. */
> + flush = (iph->ttl ^ iph2->ttl) | (iph->tos ^ iph2->tos) | (df ^ (id2 &
> IP_DF));
> +
> + /* When we receive our second frame we can make a decision on if we
> + * continue this flow as an atomic flow with a fixed ID or if we use
> + * an incremdfenting ID.
> + */
Comment became garbled on move: incrementing
> + if (count == 1) {
> + is_atomic = df && flush_id == 0;
> + NAPI_GRO_CB(p)->is_atomic = is_atomic;
> + } else {
> + is_atomic = df && NAPI_GRO_CB(p)->is_atomic;
> + }
> +
> + /* Ignore outer IP ID value if based on atomic datagram. */
> + outer = (outer && df) - 1;
> + is_atomic--;
> +
> + return flush | ((flush_id ^ (count & is_atomic)) & outer);
> +}
Re: [PATCH] tools/testing/selftests/bpf/test_tc_tunnel.sh: Prevent client connect before server bind
Hi Martin,
Thanks for the review.
Il giorno ven 8 mar 2024 alle ore 02:03 Martin KaFai Lau
ha scritto:
>
> On 2/29/24 6:00 AM, Alessandro Carminati (Red Hat) wrote:
> > In some systems, the netcat server can incur in delay to start listening.
> > When this happens, the test can randomly fail in various points.
> > This is an example error message:
> > # ip gre none gso
> > # encap 192.168.1.1 to 192.168.1.2, type gre, mac none len 2000
> > # test basic connectivity
> > # Ncat: Connection refused.
>
> This explained what is the issue. Please also explain how the patch solves it.
>
The issue, as stated, depends on a race condition between the netcat client
and server. The test author addressed this problem using a sleep, which I
removed in this patch. To easily solve the issue, one could simply increase
the sleep duration. However, this patch opts to tackle the problem by
querying the /proc directory and verifying TCP binds at the specified port
before letting the client connect.
> >
> > Signed-off-by: Alessandro Carminati (Red Hat)
> >
> > ---
> > tools/testing/selftests/bpf/test_tc_tunnel.sh | 19 ++-
> > 1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_tc_tunnel.sh
> > b/tools/testing/selftests/bpf/test_tc_tunnel.sh
> > index 910044f08908..01c0f4b1a8c2 100755
> > --- a/tools/testing/selftests/bpf/test_tc_tunnel.sh
> > +++ b/tools/testing/selftests/bpf/test_tc_tunnel.sh
> > @@ -72,7 +72,6 @@ cleanup() {
> > server_listen() {
> > ip netns exec "${ns2}" nc "${netcat_opt}" -l "${port}" > "${outfile}"
> > &
> > server_pid=$!
> > - sleep 0.2
> > }
> >
> > client_connect() {
> > @@ -93,6 +92,22 @@ verify_data() {
> > fi
> > }
> >
> > +wait_for_port() {
> > + local digits=8
> > + local port2check=$(printf ":%04X" $1)
> > + local prot=$([ "$2" == "-6" ] && echo 6 && digits=32)
> > +
> > + for i in $(seq 20); do
> > + if ip netns exec "${ns2}" cat /proc/net/tcp${prot} | \
> > + sed -r 's/^[ \t]+[0-9]+:
> > ([0-9A-F]{'${digits}'}:[0-9A-F]{4}) .*$/\1/' | \
> > + grep -q "${port2check}"; then
>
> The idea is to check if there is socket listening on port ?
>
> May be something simpler like "ss -OHtl src :$1" instead?
Indeed, the aim is to ensure that the server is bound before the
client attempts to
connect by checking if socket is listening, and yes using 'ss' would be shorter.
However, I chose not to use 'ss' or 'netstat' to avoid adding new dependencies,
considering they are already many.
Nonetheless, 'ss' is preferred, I have no objections.
>
> --
> pw-bot: cr
>
> The check-and-wait fix in this patch is fine to get your test environment
> going.
>
> Eventually, it will be good to see the test_tc_tunnel.sh test moved to
> test_progs. The test_tc_tunnel.sh is not run by bpf CI and issue like this got
> unnoticed. Some other "*.sh" tests have already been moved to test_progs.
>
>
Regards
Alessandro
