On 7/10/2018 4:40 PM, Gaëtan Rivet wrote:
On Tue, Jul 10, 2018 at 04:22:23PM +0800, Jeff Guo wrote:
On 7/9/2018 9:48 PM, Andrew Rybchenko wrote:
On 09.07.2018 15:01, Jeff Guo wrote:
This patch aim to add a helper to iterate all buses to find the
corresponding bus to handle the sigbus error.
Signed-off-by: Jeff Guo <jia....@intel.com>
Acked-by: Shaopeng He <shaopeng...@intel.com>
---
v7->v6:
no change
---
lib/librte_eal/common/eal_common_bus.c | 42
++++++++++++++++++++++++++++++++++
lib/librte_eal/common/eal_private.h | 12 ++++++++++
2 files changed, 54 insertions(+)
diff --git a/lib/librte_eal/common/eal_common_bus.c
b/lib/librte_eal/common/eal_common_bus.c
index 0943851..8856adc 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -37,6 +37,7 @@
#include <rte_bus.h>
#include <rte_debug.h>
#include <rte_string_fns.h>
+#include <rte_errno.h>
#include "eal_private.h"
@@ -242,3 +243,44 @@ rte_bus_get_iommu_class(void)
}
return mode;
}
+
+static int
+bus_handle_sigbus(const struct rte_bus *bus,
+ const void *failure_addr)
+{
+ int ret;
+
+ if (!bus->sigbus_handler) {
+ RTE_LOG(ERR, EAL, "Function sigbus_handler not supported by "
+ "bus (%s)\n", bus->name);
It is not an error. It is OK that some buses cannot handle SIGBUS.
yes, it is.
+ return -1;
+ }
+
+ ret = bus->sigbus_handler(failure_addr);
+ rte_errno = ret;
+
+ return !(bus->sigbus_handler && ret <= 0);
There is no point to check bus->sigbus_handler here. It is already
checked above.
So, it should be just:
return ret > 0;
I.e. we should continue search if the address is not handled by any
device
on the bus (we should stop if it is handled (ret==0) or failed to to
handle
(ret < 0)).
i will modify it, thanks.
Why is rte_errno set here?
rte_errno is meant by the bus dev to be set on error. You do not have to
modify it.
ret would already be <0 on error.
At most, you could do something like:
if (ret < 0 && rte_errno == 0)
rte_errno = ENOTSUP;
Or something akin, with a non-descriptive error hinting that the
developper didn't seem to care about setting errno to something
meaningful (so only partially respecting the API).
the purpose to set rte_errno here is because of the status of the
handle need to pass though to the function caller "rte_bus_sigbus_handler",
it could give a chance to check the searching status.
+}
+
+int
+rte_bus_sigbus_handler(const void *failure_addr)
+{
+ struct rte_bus *bus;
+
+ int ret = 0;
+ int old_errno = rte_errno;
+
+ rte_errno = 0;
+
+ bus = rte_bus_find(NULL, bus_handle_sigbus, failure_addr);
+ /* failed to handle the sigbus, pass the new errno. */
+ if (!bus)
+ ret = 1;
+ else if (rte_errno == -1)
I'm still thinking it is bad to keep negative value in rte_errno here.
i think the rte_errno just no used for the caller if return -1. Since if
find bus but process failed, will use rte_exit to process whatever the
rte_errno value. Only return 1 means use the origin sigbus handler that will
care about the errno.
With the changes above, the check should be something like:
if (bus == NULL)
return 1;
else if (rte_errno != 0)
return -rte_errno;
rte_errno = old_errno;
return 0;
Which would avoid resetting rte_errno on top of whichever value a dev
would have used, and having it set to a negative non-errno value.
(Please do not just use this as-is, if you think this is not a good idea
just tell us why or how you would prefer to do it. I'm only proposing a
way that I think would work.)
Regards,
i think that is the problem to find a better way, i agree to maximum to
keep the rte_errno should be make sense.
