On Mon, 23 Jul 2018 11:41:02 +0200 Björn Töpel <bjorn.to...@gmail.com> wrote:
> > >> diff --git a/net/core/xdp.c b/net/core/xdp.c > > >> index 9d1f220..1c12bc7 100644 > > >> --- a/net/core/xdp.c > > >> +++ b/net/core/xdp.c > > >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct > > >> xdp_mem_info *mem, bool napi_direct, > > >> rcu_read_lock(); > > >> /* mem->id is valid, checked in > > >> xdp_rxq_info_reg_mem_model() */ > > >> xa = rhashtable_lookup(mem_id_ht, &mem->id, > > >> mem_id_rht_params); > > >> - xa->zc_alloc->free(xa->zc_alloc, handle); > > >> + if (xa) > > >> + xa->zc_alloc->free(xa->zc_alloc, handle); > > > hmm...It is not clear to me the "!xa" case don't have to be handled? > > > > Thank you for reviewing! > > > > Returning NULL pointer is bug case such as calling after use > > xdp_rxq_info_unreg(). > > so that, I think it can't handle at that moment. > > we can make __xdp_return to add WARN_ON_ONCE() or > > add return error code to driver. > > But I'm not sure if these is useful information. > > > > I might have misunderstood scenario of MEM_TYPE_ZERO_COPY > > because there is no use case of MEM_TYPE_ZERO_COPY yet. > > > > Taehee, again, sorry for the slow response and thanks for patch! > > If xa is NULL, the driver has a buggy/broken implementation. What > would be a proper way of dealing with this? BUG? Hmm... I don't like these kind of changes to the hot-path code! You might not realize this, but adding BUG() and WARN_ON() to the code affect performance in ways you might not realize! These macros gets compiled and uses an asm instruction called "ud2". Seeing the "ud2" instruction causes the CPUs instruction cache prefetcher to stop. Thus, if some code ends up below this instruction, this will cause more i-cache-misses. I don't know if xa==NULL is even possible, but if it is, then I think this is a result of a driver mem_reg API usage bug. And the mem-reg API is full of WARN's and error messages, exactly to push these kind of checks out of the fast-path. There is no need for a BUG() call, as deref a NULL pointer will case an OOPS, that is easy to read and understand. -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
