[dpdk-dev] [PATCH v7 5/7] bus: add helper to handle sigbus

[Gaëtan Rivet]

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.guo at intel.com>
> > > Acked-by: Shaopeng He <shaopeng.he at 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).

> > > +}
> > > +
> > > +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,
-- 
Gaëtan Rivet
6WIND