Den ons 1 aug. 2018 kl 16:14 skrev Jesper Dangaard Brouer <bro...@redhat.com>: > > 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. >
Jesper, thanks for having a look! So, you're right that if xa==NULL the driver is "broken/buggy" (as stated earlier!). I agree that OOPSing on a NULL pointer is as good as a BUG! The applied patch adds a WARN_ON_ONCE, and I thought best practice was that a buggy driver shouldn't crash the kernel... What is considered best practices in these scenarios? *I'd* prefer an OOPS instead of WARN_ON_ONCE, to catch that buggy driver. Again, that's me. I thought that most people prefer not crashing, hence the patch. :-) Björn > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer
